feat: enhance filename sanitization and security checks to prevent path traversal attacks#111
feat: enhance filename sanitization and security checks to prevent path traversal attacks#111
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 22 minutes and 30 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughA VS Code workspace configuration file was added, and the filename handling in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
helpers/write_apkg.py (1)
77-144:⚠️ Potential issue | 🟠 Major
rename_temp_fileis over the local-variable threshold and duplicates safety logic.
pylint (too-many-locals)is failing. Extract path-validation/path-build into a small helper and reuse it for main + fallback branches to reduce locals and keep the safety checks consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helpers/write_apkg.py` around lines 77 - 144, The rename_temp_file function has too many local variables and duplicates path-safety checks; extract a small helper (e.g., build_safe_final_path or validate_and_build_path) that takes name and deck_id (and optionally a fallback basename) and returns a validated final_path (performing basename extraction, dangerous pattern check, length truncation, os.path.join with cwd, realpath containment check, and final basename strip), then call that helper for the main filename and again for the fallback short_final_filename inside the OSError handler; update rename_temp_file to use the helper to reduce locals and remove duplicated safety logic while preserving the same exceptions and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@helpers/write_apkg.py`:
- Line 92: Remove trailing whitespace characters from the affected lines in
helpers/write_apkg.py (e.g., inside the write_apkg-related
functions/module-level code where blank/comment lines currently end with
spaces); strip the trailing spaces on those lines (and any other lines with
trailing whitespace in that file), save, and re-run lint to confirm pylint:
trailing-whitespace is resolved.
- Around line 133-137: When validating the fallback filename in the exception
handler, preserve the original OSError context by chaining the new ValueError to
the caught exception (variable e); locate the block that computes
real_final_path from final_path and checks startswith against real_cwd inside
the except OSError as e handler, and change the raise of ValueError("Path
traversal attempt detected in fallback filename") to raise that ValueError from
e so the traceback shows the original rename failure.
- Around line 121-122: Replace the brittle startswith containment check that
uses real_cwd + os.sep with an os.path.commonpath-based check using the existing
real_cwd and real_final_path variables; specifically, in the blocks around the
checks (the ones currently using real_final_path.startswith(real_cwd + os.sep)
and the identical check later), validate containment with if
os.path.commonpath([real_cwd, real_final_path]) != real_cwd: raise
ValueError("Path traversal attempt detected in final path"). Ensure real_cwd and
real_final_path are absolute/normalized beforehand (they already are in this
module) and apply the same replacement to both occurrences referenced in the
diff.
---
Outside diff comments:
In `@helpers/write_apkg.py`:
- Around line 77-144: The rename_temp_file function has too many local variables
and duplicates path-safety checks; extract a small helper (e.g.,
build_safe_final_path or validate_and_build_path) that takes name and deck_id
(and optionally a fallback basename) and returns a validated final_path
(performing basename extraction, dangerous pattern check, length truncation,
os.path.join with cwd, realpath containment check, and final basename strip),
then call that helper for the main filename and again for the fallback
short_final_filename inside the OSError handler; update rename_temp_file to use
the helper to reduce locals and remove duplicated safety logic while preserving
the same exceptions and behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1fb98004-64c3-42d2-9d4a-bac344da6f34
📒 Files selected for processing (3)
.vscode/settings.jsonhelpers/write_apkg.pytests/test_write_apkg.py
| # Check for path traversal attempts in the original inputs | ||
| name_str = str(sanitized_name) | ||
| id_str = str(first_deck_id) | ||
|
|
There was a problem hiding this comment.
Remove trailing whitespace to unblock lint CI.
These lines currently fail the pipeline (pylint: trailing-whitespace).
Also applies to: 98-98, 102-102, 112-112, 116-116, 120-120, 132-132, 137-137
🧰 Tools
🪛 GitHub Actions: Lint
[error] 92-92: pylint (trailing-whitespace): Trailing whitespace
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@helpers/write_apkg.py` at line 92, Remove trailing whitespace characters from
the affected lines in helpers/write_apkg.py (e.g., inside the write_apkg-related
functions/module-level code where blank/comment lines currently end with
spaces); strip the trailing spaces on those lines (and any other lines with
trailing whitespace in that file), save, and re-run lint to confirm pylint:
trailing-whitespace is resolved.
| if not real_final_path.startswith(real_cwd + os.sep) and not real_final_path == real_cwd: | ||
| raise ValueError("Path traversal attempt detected in final path") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n helpers/write_apkg.py | head -150Repository: 2anki/create_deck
Length of output: 6088
🏁 Script executed:
sed -n '110,140p' helpers/write_apkg.pyRepository: 2anki/create_deck
Length of output: 1526
🏁 Script executed:
sed -n '90,130p' helpers/write_apkg.pyRepository: 2anki/create_deck
Length of output: 1779
🏁 Script executed:
python3 << 'EOF'
import os
import tempfile
# Test the original logic with root directory
real_cwd = "/"
test_paths = [
"/tmp/file.txt", # Valid: under root
"/home/user/file.txt", # Valid: under root
"/", # Valid: the root itself
"/../etc/passwd", # Should reject traversal
]
print("Testing original logic: startswith(real_cwd + os.sep) and real_final_path == real_cwd")
print(f"real_cwd = '{real_cwd}'")
print(f"real_cwd + os.sep = '{real_cwd + os.sep}'")
print()
for path in test_paths:
# Normalize path like the code would
real_final_path = os.path.realpath(path)
original_check = not real_final_path.startswith(real_cwd + os.sep) and not real_final_path == real_cwd
print(f"Path: {path}")
print(f" Resolved to: {real_final_path}")
print(f" Original logic rejects: {original_check}")
print()
print("\n" + "="*60)
print("Testing suggested fix: os.path.commonpath([real_cwd, real_final_path]) != real_cwd")
print()
for path in test_paths:
real_final_path = os.path.realpath(path)
suggested_check = os.path.commonpath([real_cwd, real_final_path]) != real_cwd
print(f"Path: {path}")
print(f" Resolved to: {real_final_path}")
print(f" Suggested logic rejects: {suggested_check}")
print()
print("\n" + "="*60)
print("Testing with non-root CWD:")
real_cwd = "/home/user"
print(f"real_cwd = '{real_cwd}'")
print(f"real_cwd + os.sep = '{real_cwd + os.sep}'")
print()
test_paths_nonroot = [
"/home/user/file.txt", # Valid
"/home/user", # Valid: the cwd itself
"/home/other/file.txt", # Invalid: outside cwd
]
for path in test_paths_nonroot:
real_final_path = os.path.realpath(path)
original_check = not real_final_path.startswith(real_cwd + os.sep) and not real_final_path == real_cwd
suggested_check = os.path.commonpath([real_cwd, real_final_path]) != real_cwd
print(f"Path: {path}")
print(f" Resolved to: {real_final_path}")
print(f" Original rejects: {original_check}, Suggested rejects: {suggested_check}")
print()
EOFRepository: 2anki/create_deck
Length of output: 1436
🏁 Script executed:
python3 << 'EOF'
import os
# Check potential issues with commonpath
print("Testing edge cases with os.path.commonpath:")
print()
# Case 1: Symlink escapes
print("Case 1: Path with similar prefix but different meaning")
paths_to_test = [
("/home/user", "/home/user_backup"),
("/home/user", "/home/username"),
("/app", "/apple"),
]
for cwd, final in paths_to_test:
common = os.path.commonpath([cwd, final])
check = common != cwd
print(f"CWD: {cwd}, Path: {final}")
print(f" Common path: {common}")
print(f" Would reject: {check}")
print()
# Case 2: Root directory edge case
print("Case 2: Root directory behavior")
print(f"os.path.commonpath(['/'], '/tmp/file'): {os.path.commonpath(['/'], '/tmp/file')}")
print(f"os.path.commonpath(['/'], '/'): {os.path.commonpath(['/'], '/')}")
print()
# Case 3: What if paths are on different drives (Windows)?
# This is less relevant on Linux but good to know
print("Case 3: Non-existent paths")
try:
result = os.path.commonpath(["/nonexistent", "/nonexistent/file"])
print(f"commonpath with non-existent paths works: {result}")
except Exception as e:
print(f"commonpath with non-existent paths fails: {e}")
EOFRepository: 2anki/create_deck
Length of output: 580
Path containment check fails when CWD is root (/).
When real_cwd is /, the expression real_cwd + os.sep produces //, causing startswith() to incorrectly reject all valid paths like /tmp/file.txt. Use os.path.commonpath([real_cwd, real_final_path]) != real_cwd instead, which correctly validates path containment and also prevents prefix-matching attacks (e.g., distinguishing /home/user from /home/user_backup).
Suggested fix
- if not real_final_path.startswith(real_cwd + os.sep) and not real_final_path == real_cwd:
+ if os.path.commonpath([real_cwd, real_final_path]) != real_cwd:
raise ValueError("Path traversal attempt detected in final path")
...
- if not real_final_path.startswith(real_cwd + os.sep) and not real_final_path == real_cwd:
+ if os.path.commonpath([real_cwd, real_final_path]) != real_cwd:
raise ValueError("Path traversal attempt detected in fallback filename")Applies to lines 121-122 and 135-136.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 121-121: Use the opposite operator ("!=") instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@helpers/write_apkg.py` around lines 121 - 122, Replace the brittle startswith
containment check that uses real_cwd + os.sep with an os.path.commonpath-based
check using the existing real_cwd and real_final_path variables; specifically,
in the blocks around the checks (the ones currently using
real_final_path.startswith(real_cwd + os.sep) and the identical check later),
validate containment with if os.path.commonpath([real_cwd, real_final_path]) !=
real_cwd: raise ValueError("Path traversal attempt detected in final path").
Ensure real_cwd and real_final_path are absolute/normalized beforehand (they
already are in this module) and apply the same replacement to both occurrences
referenced in the diff.
…vent traversal attacks
|
1 participant
Summary by CodeRabbit