[feature] Improved security, performance, test coverage, agents#5
[feature] Improved security, performance, test coverage, agents#5nemesifier merged 6 commits intomasterfrom
Conversation
|
Warning Rate limit exceeded
⌛ 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. 📒 Files selected for processing (1)
WalkthroughThis PR introduces security hardening and resource management to the django-minify-compress-staticfiles project. It adds path traversal protection via Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ 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 |
Pull Request Test Coverage Report for Build 21549245964Details
💛 - Coveralls |
nemesifier
force-pushed
the
security-perofmrance
branch
from
7476b53 to
4cd30ce
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
django_minify_compress_staticfiles/storage.py (2)
173-184:⚠️ Potential issue | 🔴 CriticalAbsolute-path normalization should validate against storage base to prevent path injection.
The current code uses
parts[1:]to strip the root directory, which converts/etc/passwdtoetc/passwd. This path is then embedded under the storage root without validation, allowing arbitrary absolute paths to be written into the storage directory. The approach also fails to detect or prevent path traversal attempts.Use
os.path.relpath(path, base_dir)with validation: paths that escape the storage base (detected bystartswith("..")) should be skipped rather than processed. This properly relativizes paths within the storage boundary while preventing directory traversal attacks.🔧 Suggested fix
- if os.path.isabs(path): - path_obj = Path(path) - parts = path_obj.parts - # parts[0] is the root/drive (e.g., "/" or "C:\\"); join the remaining parts - if len(parts) > 1: - relative_path = os.path.join(*parts[1:]) - else: - # Fallback: if for some reason there are no extra parts, use the basename - relative_path = os.path.basename(path) + if os.path.isabs(path): + base_dir = getattr(self, "location", None) or getattr( + self, "base_location", None + ) + if base_dir: + relative_path = os.path.relpath(path, base_dir) + if relative_path.startswith(".."): + logger.warning( + f"Skipping path outside storage base: {path}" + ) + continue + else: + # Fallback if no base directory is available + relative_path = os.path.basename(path) else: relative_path = path
211-258:⚠️ Potential issue | 🟠 MajorBound reads to MAX_FILE_SIZE and enforce base-dir safety for absolute paths.
f.read()loads the full blob before the size check, which can be expensive for large files. Also,is_safe_path(path)without a base allows absolute paths outside the storage root to pass through unchecked—paths like/etc/passwdvalidate as safe.🔧 Suggested fix
- if not is_safe_path(path): - logger.warning(f"Skipping unsafe path: {path}") - return None + base_dir = getattr(self, "location", None) or getattr(self, "base_location", None) + if os.path.isabs(path): + if base_dir and not is_safe_path(path, base_dir=base_dir): + logger.warning(f"Skipping unsafe path: {path}") + return None + elif not is_safe_path(path): + logger.warning(f"Skipping unsafe path: {path}") + return None @@ - with self.open(path) as f: - content = f.read() + with self.open(path) as f: + content = f.read(max_size + 1) if isinstance(content, bytes) and len(content) > max_size: logger.warning(f"File too large, skipping: {path}") return None return content @@ - if not is_safe_path(path): - logger.warning(f"Skipping unsafe path for writing: {path}") - return + base_dir = getattr(self, "location", None) or getattr(self, "base_location", None) + if os.path.isabs(path): + if base_dir and not is_safe_path(path, base_dir=base_dir): + logger.warning(f"Skipping unsafe path for writing: {path}") + return + elif not is_safe_path(path): + logger.warning(f"Skipping unsafe path for writing: {path}") + return
🤖 Fix all issues with AI agents
In `@django_minify_compress_staticfiles/utils.py`:
- Around line 48-57: The current logic reads the whole file into memory via
f.read() even when oversized; change it to check size before loading (use
os.path.getsize(file_path) against max_size) or read only max_size+1 bytes
(f.read(max_size+1)) to detect oversize, then return "" and logger.warning if
too big; if within limits, compute the hash—preferably by streaming into
hashlib.sha256() in chunks instead of f.read() to avoid loading large files at
once. Ensure you reference and use the existing variables file_path, max_size,
length, logger, and hashlib.sha256 when implementing the fix.
In `@tests/test_edge_cases.py`:
- Around line 6-12: The test module imports unused symbols causing F401; remove
the unused imports override_settings and FileManager from the import list so
only is_safe_path and should_process_file are imported from
django_minify_compress_staticfiles.utils; update the top-of-file import
statement accordingly to eliminate the unused names (override_settings,
FileManager) and fix the flake8 failure.
- Around line 108-116: Rename the unused parameter in the test double to silence
ARG002: update TestFileManager.is_compression_candidate to accept _file_path
(instead of file_path) so intent remains clear and the linter won't flag the
unused argument; adjust any local references (none expected) accordingly.
In `@tests/test_security.py`:
- Around line 14-20: The import list in tests imports an unused symbol
FileProcessorMixin which triggers flake8 F401; remove FileProcessorMixin from
the import statement (the line that currently imports CompressionMixin,
FileProcessorMixin, MinicompressStorage, MinificationMixin) so only the used
symbols remain, then run linters/tests to confirm the CI failure is resolved;
keep the other imports (CompressionMixin, MinicompressStorage,
MinificationMixin) and imported utils (FileManager, is_safe_path) unchanged.
In `@tests/test_utils.py`:
- Line 6: The top-level import FileSystemStorage is unused and shadowed by a
local definition in setUp, causing flake8 F401/F811; remove the module-level
"from django.core.files.storage import FileSystemStorage" import and rely on the
local FileSystemStorage created/assigned inside the TestCase.setUp (or import it
only where needed), ensuring no duplicate definitions remain (refer to the
FileSystemStorage symbol and the setUp method to locate and validate the
change).
🧹 Nitpick comments (5)
django_minify_compress_staticfiles/__init__.py (1)
11-11: Minor: Redundant coverage exclusion.The
# pragma: no coveris redundant since.coveragercalready omits this file via both*/__init__.pyand the explicitdjango_minify_compress_staticfiles/__init__.pyentry. Not harmful, but one exclusion mechanism would suffice..coveragerc (1)
7-10: Minor: Redundant omit pattern.Line 10 (
django_minify_compress_staticfiles/__init__.py) is already covered by the glob pattern on line 7 (*/__init__.py). Consider removing the explicit entry to reduce duplication.♻️ Proposed simplification
omit = */test* */__init__.py */migrations/* */tests/* - django_minify_compress_staticfiles/__init__.pytests/test_security.py (3)
23-25: Empty test class - remove or implement.
SecurityTestshas no test methods. Either add tests or remove the class to avoid confusion about test coverage.
366-396: Redundant cleanup withTemporaryDirectorycontext manager.Line 395 calls
shutil.rmtree(temp_dir, ...)but theTemporaryDirectorycontext manager on line 373 already handles cleanup automatically when exiting thewithblock.🧹 Proposed fix
# Manifest should have original entry but not new one with open(manifest_path, "r") as f: manifest = json.load(f) self.assertIn("existing.css", manifest) self.assertNotIn("new.css", manifest) - - shutil.rmtree(temp_dir, ignore_errors=True)
411-433: Dead code and unused parameter cleanup.
- Line 414:
pathparameter is unused inmock_exists. Prefix with underscore to indicate intentionally unused.- Line 432:
original_openis never modified, so restoring it is unnecessary.🧹 Proposed fix
original_exists = self.processor.exists - original_open = self.processor.open - def mock_exists(path): + def mock_exists(_path): # Return False to force fallback to filesystem return False self.processor.exists = mock_exists try: # Create a file in the temp dir test_file = os.path.join(self.processor.temp_dir, "test.txt") with open(test_file, "w") as f: f.write("test") # Now mock os.path.getsize to raise OSError with patch("os.path.getsize", side_effect=OSError("Permission denied")): result = self.processor._read_file_content(test_file) self.assertIsNone(result) finally: self.processor.exists = original_exists - self.processor.open = original_open
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.coveragerc.gitignoreAGENTS.mdREADME.rstdjango_minify_compress_staticfiles/__init__.pydjango_minify_compress_staticfiles/conf.pydjango_minify_compress_staticfiles/storage.pydjango_minify_compress_staticfiles/utils.pytest_staticfiles/staticfiles.jsontests/test_basic.pytests/test_conf.pytests/test_edge_cases.pytests/test_security.pytests/test_storage.pytests/test_utils.py
🧰 Additional context used
🧬 Code graph analysis (5)
tests/test_security.py (4)
django_minify_compress_staticfiles/storage.py (12)
CompressionMixin(145-293)FileProcessorMixin(21-76)MinicompressStorage(297-355)MinificationMixin(79-142)process_minification(82-142)process_compression(148-207)minify_file_content(46-76)gzip_compress(264-279)brotli_compress(281-293)_read_file_content(209-238)_write_file_content(240-262)_update_manifest(335-355)django_minify_compress_staticfiles/utils.py (3)
FileManager(122-171)is_safe_path(14-30)should_process(148-159)tests/test_storage.py (11)
exists(26-29)path(56-57)open(31-33)save(35-54)cleanup(59-60)setUp(84-85)setUp(166-167)setUp(236-237)setUp(293-296)cssmin(128-129)jsmin(150-151)tests/test_utils.py (1)
setUp(143-148)
tests/test_utils.py (1)
django_minify_compress_staticfiles/utils.py (8)
FileManager(122-171)create_hashed_filename(68-82)generate_file_hash(39-65)get_file_size(113-119)is_safe_path(14-30)normalize_extension(85-87)should_process_file(90-110)validate_file_size(33-36)
tests/test_edge_cases.py (1)
django_minify_compress_staticfiles/utils.py (7)
FileManager(122-171)is_safe_path(14-30)should_process_file(90-110)supported_extensions(129-134)exclude_patterns(137-140)min_file_size(143-146)is_compression_candidate(161-171)
django_minify_compress_staticfiles/storage.py (2)
django_minify_compress_staticfiles/utils.py (2)
generate_file_hash(39-65)is_safe_path(14-30)django_minify_compress_staticfiles/conf.py (1)
get_setting(4-6)
django_minify_compress_staticfiles/utils.py (1)
django_minify_compress_staticfiles/conf.py (1)
get_setting(4-6)
🪛 GitHub Actions: CI
tests/test_security.py
[error] 14-14: flake8: F401 'django_minify_compress_staticfiles.storage.FileProcessorMixin' imported but unused
tests/test_utils.py
[error] 6-6: flake8: F401 'django.core.files.storage.FileSystemStorage' imported but unused
[error] 145-145: flake8: F811 redefinition of unused 'FileSystemStorage' from line 6
tests/test_edge_cases.py
[error] 6-6: flake8: F401 'django.test.override_settings' imported but unused
[error] 8-8: flake8: F401 'django_minify_compress_staticfiles.utils.FileManager' imported but unused
🪛 LanguageTool
AGENTS.md
[uncategorized] ~79-~79: The official name of this software platform is spelled with a capital “H”.
Context: ... reportlocally. - CI issues: Refer to.github/workflows/ci.yml`.
(GITHUB)
🪛 Ruff (0.14.14)
tests/test_security.py
[warning] 414-414: Unused function argument: path
(ARG001)
tests/test_edge_cases.py
[warning] 115-115: Unused method argument: file_path
(ARG002)
django_minify_compress_staticfiles/storage.py
[warning] 59-59: Do not catch blind exception: Exception
(BLE001)
[warning] 60-60: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
[warning] 204-204: Do not catch blind exception: Exception
(BLE001)
[warning] 205-205: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
[warning] 236-236: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
tests/test_storage.py
[warning] 128-128: Unused static method argument: content
(ARG004)
[warning] 128-128: Unused static method argument: keep_bang_comments
(ARG004)
[warning] 129-129: Create your own exception
(TRY002)
[warning] 129-129: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 150-150: Unused static method argument: content
(ARG004)
[warning] 150-150: Unused static method argument: keep_bang_comments
(ARG004)
[warning] 151-151: Create your own exception
(TRY002)
[warning] 151-151: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 377-377: Unused function argument: content
(ARG001)
[warning] 377-377: Unused function argument: file_type
(ARG001)
[warning] 378-378: Create your own exception
(TRY002)
[warning] 378-378: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 430-430: Unused function argument: content
(ARG001)
[warning] 431-431: Create your own exception
(TRY002)
[warning] 431-431: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 515-515: Unused function argument: name
(ARG001)
[warning] 515-515: Unused function argument: content
(ARG001)
[warning] 516-516: Create your own exception
(TRY002)
[warning] 516-516: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (35)
.gitignore (1)
27-27: LGTM!Adding
coverage.xmlto.gitignoreis appropriate as it's a generated CI artifact that should not be tracked in version control.test_staticfiles/staticfiles.json (1)
1-1: LGTM!Empty JSON object is a valid test fixture for manifest handling tests.
AGENTS.md (1)
1-79: LGTM!Comprehensive contributor guide with clear instructions for setup, testing, QA, and security considerations. The
is_safe_path()mention in the code review checklist aligns well with the security features introduced in this PR.Note: The static analysis hint about "GitHub" capitalization on line 79 is a false positive—
.githubis the correct lowercase filesystem path for GitHub Actions workflows.README.rst (1)
86-197: LGTM!Excellent documentation additions:
- Clear explanations of new security-related settings (
MAX_FILE_SIZE,MAX_FILES_PER_RUN)- Well-reasoned compression level defaults with performance trade-off explanations
- Comprehensive Security Considerations section covering path traversal, memory/CPU exhaustion, and compression bomb protection
- Practical recommended settings for production vs development environments
django_minify_compress_staticfiles/conf.py (2)
15-19: LGTM!Good security and performance defaults:
MAX_FILE_SIZE(10MB) prevents memory exhaustion from oversized filesMAX_FILES_PER_RUN(1000) caps processing to prevent CPU exhaustion- Reduced compression levels (Gzip: 6, Brotli: 4) provide better CPU/compression trade-off
32-38: LGTM!Extending
EXCLUDE_PATTERNSwith*.gz,*.br, and*.zipprevents double-compression of already compressed files, which is both a performance optimization and security measure against compression bombs.tests/test_conf.py (1)
41-44: Default setting assertions updated correctly.
Covers MAX_FILE_SIZE and the new compression level defaults.tests/test_edge_cases.py (2)
15-67: Solid edge-case coverage foris_safe_path.
Empty, backslash, drive, and symlink scenarios are well exercised.
69-97: Edge-case coverage forshould_process_filelooks good.
Nice inclusion of None/empty patterns and case normalization.tests/test_basic.py (1)
109-112: Updated default expectations look correct.
Matches MAX_FILE_SIZE and revised compression level defaults.tests/test_utils.py (5)
54-65: Large-file hashing test is a good guardrail.
Covers the MAX_FILE_SIZE behavior clearly.
82-87: Hash replacement test is spot-on.
Confirms existing hashes are replaced correctly.
127-138: Exclude-pattern suffix cases are well covered.
Both wildcard and plain suffix behavior are tested.
177-213:is_safe_pathbehavior tests are comprehensive.
Good coverage of traversal, safe paths, and base_dir checks.
214-226:validate_file_sizetests look good.
Covers in-range and oversized values.django_minify_compress_staticfiles/utils.py (1)
14-36: New safety/size helpers look good.
Clear, minimal validation logic.tests/test_storage.py (5)
69-71: Mixin stack update for testable minification is appropriate.
Enables compression paths in the minification test harness.
118-161: Minification exception-handling tests are valuable.
Nice coverage of CSS/JS error fallbacks.
163-231: Minification edge cases and limits are well tested.
Covers unchanged content, directories, binaries, limits, and unsafe paths.
260-288: Compression edge cases are solid.
Absolute paths and read fallbacks are exercised cleanly.
333-524: Expanded MinicompressStorage tests add strong safety coverage.
Good breadth across manifest updates, size limits, unsafe paths, and dry-run behavior.django_minify_compress_staticfiles/storage.py (3)
50-72: Preserve-comments handling is clear.
Good use of PRESERVE_COMMENTS to set keep_bang_comments.
87-139: Per-run cap and hashed naming look good.
Limits are enforced and hashes are derived from path+content.
267-290: Compression-level clamping looks good.
Ensures valid ranges while honoring defaults.tests/test_security.py (11)
27-63: LGTM!The
_TestableSecurityMinificationhelper class correctly implements the storage interface needed for testing security features, following the same pattern used intest_storage.py.
65-93: LGTM!Good coverage of path traversal protection including relative paths, nested traversals, and base directory validation with real temp directories.
95-124: LGTM!Tests correctly verify MAX_FILE_SIZE enforcement for both exceeding and within-limit scenarios.
126-155: LGTM!Tests properly verify
MAX_FILES_PER_RUNlimit enforcement for both minification and compression workflows.
157-194: LGTM!Edge case tests for minification correctly verify exception handling and the
PRESERVE_COMMENTS=Nonebranch.
196-245: LGTM!Excellent compression edge case coverage including string input handling and compression level clamping for both out-of-range and negative values.
247-279: LGTM!Tests correctly verify absolute path handling during compression processing.
281-349: LGTM!Comprehensive tests for unsafe path rejection in both read and write operations, plus good coverage of the fallback mechanism when the
savemethod is unavailable.
351-364: LGTM!Clear and focused test for
FileManager.should_processextension filtering.
435-462: LGTM!Good coverage of binary content handling including invalid UTF-8 sequences and bytes input to minification.
464-483: LGTM!Test correctly verifies that files with
Nonecontent are skipped during compression.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/test_edge_cases.py`:
- Around line 24-36: Remove the silent except in
test_is_safe_path_different_drives and make the test explicitly assert the
expected behavior: call is_safe_path(r"D:\file.txt", base_dir=r"C:\base") and
assertFalse on its return value (do not swallow ValueError). If is_safe_path
currently raises ValueError for different-drive paths on Windows, modify the
is_safe_path function to catch that ValueError (from os.path.relpath or
equivalent) and return False instead so the test consistently passes; reference
the is_safe_path function and the test_is_safe_path_different_drives test when
making changes.
🧹 Nitpick comments (2)
tests/test_edge_cases.py (2)
52-60: Acknowledged symlink limitation has security implications.The test documents that
is_safe_pathdoesn't resolve symlinks, meaning a symlink pointing to..passes the check. If this function guards against path traversal attacks, adversaries could potentially exploit symlinks. Consider whether symlink resolution viaos.path.realpath()should be added tois_safe_path, or document this limitation clearly in the function's docstring.
95-115: Test doesn't exercise the realFileManagerclass.
TestFileManageris a local mock that doesn't test the actualFileManagerfromdjango_minify_compress_staticfiles.utils. The assertion on line 115 trivially passes sincesupported_extensionswas explicitly set toNone. If the intent is to verify realFileManagerbehavior when initialized withsupported_extensions=None, import and instantiate the actual class.If this test is meant to document a usage pattern rather than test
FileManager, consider renaming the class and method to clarify intent (e.g.,test_handling_none_supported_extensions_pattern).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
django_minify_compress_staticfiles/utils.pytests/test_edge_cases.py
🚧 Files skipped from review as they are similar to previous changes (1)
- django_minify_compress_staticfiles/utils.py
🔇 Additional comments (2)
tests/test_edge_cases.py (2)
1-10: LGTM!Imports are clean and all are used. The previously flagged unused imports have been properly removed.
66-93: LGTM!Good coverage of edge cases: empty/None extensions, None exclude patterns, complex glob patterns, and case normalization. The assertions align with expected behavior.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
github-project-automation
bot
moved this from In progress
to Reviewer approved
in OpenWISP Priorities for next releases
github-project-automation
bot
moved this to In progress
in OpenWISP Priorities for next releases
nemesifier
moved this from Reviewer approved
to Ready for review/testing
in OpenWISP Priorities for next releases
github-project-automation
bot
moved this from Ready for review/testing
to Done
in OpenWISP Priorities for next releases
2 participants
Improved: