Adds security scanning with Bandit #4
Conversation
Implements Bandit security scanning to identify potential vulnerabilities. Includes Bandit as a development dependency and configures it to skip B101 (assert_used) warnings in test files where assertions are expected. Updates the README with instructions on how to run Bandit locally, and adds a description of basic security features. Also improves test file handling by using secure temporary file creation.
|
Warning Rate limit exceeded@acgetchell has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 14 seconds before requesting another review. ⌛ 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)
WalkthroughThe changes introduce Bandit security scanning into the project by updating development dependencies, adding configuration, and documenting its usage in the README. The GitHub Actions workflow is modified to skip specific Bandit warnings. Test files are updated to use securely generated temporary files instead of hardcoded paths, enhancing test isolation and security. Additionally, the spelling configuration file is updated with new and reordered words. Changes
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
.github/workflows/bandit.yml (1)
48-49: Fix trailing spaces and approve Bandit configuration.The Bandit configuration correctly skips B101 (assert_used) warnings, which aligns with the pyproject.toml configuration and is appropriate for test files where assertions are expected.
Fix the trailing spaces flagged by YAMLlint:
- # comma-separated list of test IDs to skip + # comma-separated list of test IDs to skip skips: B101 # skip assert_used warnings in test filesREADME.md (1)
176-213: Excellent comprehensive security documentation.The new "Security Scanning" section provides clear, actionable instructions for using Bandit and thoroughly documents the project's security features. This greatly enhances the project's security posture and developer experience.
Fix minor language issues for clarity:
-4. Run security scan on all Python files: +4. Run a security scan on all Python files:-The project's GitHub Actions workflow automatically runs Bandit security scans on all code. The configuration in `pyproject.toml` skips B101 (assert_used) warnings for test files since assertions are expected and appropriate in tests. +The project's GitHub Actions workflow automatically runs Bandit security scans on all code. The configuration in `pyproject.toml` skips B101 (assert_used) warnings for test files, since assertions are expected and appropriate in tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.github/workflows/bandit.yml(1 hunks)README.md(1 hunks)pyproject.toml(1 hunks)tests/test_calendar_analyzer.py(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_calendar_analyzer.py (1)
calendar_analyzer.py (1)
main(327-399)
🪛 LanguageTool
README.md
[uncategorized] ~198-~198: You might be missing the article “a” here.
Context: ...it -r tests/ --skip B101 4. Run security scan on all Python files: bash ...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~204-~204: Possible missing comma found.
Context: ...ps B101 (assert_used) warnings for test files since assertions are expected and appro...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 YAMLlint (1.37.1)
.github/workflows/bandit.yml
[error] 48-48: trailing spaces
(trailing-spaces)
🪛 Ruff (0.11.9)
tests/test_calendar_analyzer.py
59-59: Use a context manager for opening files
(SIM115)
81-81: Use a context manager for opening files
(SIM115)
103-103: Use a context manager for opening files
(SIM115)
161-161: Use a context manager for opening files
(SIM115)
178-178: Use a context manager for opening files
(SIM115)
481-481: Use a context manager for opening files
(SIM115)
🪛 Pylint (3.3.7)
tests/test_calendar_analyzer.py
[refactor] 59-59: Consider using 'with' for resource-allocating operations
(R1732)
[refactor] 81-81: Consider using 'with' for resource-allocating operations
(R1732)
[refactor] 103-103: Consider using 'with' for resource-allocating operations
(R1732)
[refactor] 161-161: Consider using 'with' for resource-allocating operations
(R1732)
[refactor] 178-178: Consider using 'with' for resource-allocating operations
(R1732)
[refactor] 481-482: Consider using 'with' for resource-allocating operations
(R1732)
🪛 GitHub Actions: Pylint
tests/test_calendar_analyzer.py
[error] 15-17: pylint import-error: Unable to import 'icalendar', 'pandas', and 'dateutil'.
[error] 43-43: pylint too-many-branches: Too many branches (17/12).
[error] 110-112: pylint too-many-locals, too-many-branches, too-many-statements, and too-many-nested-blocks: Too many local variables (18/15), branches (20/12), statements (52/50), and nested blocks (7/5).
[error] 198-198: pylint too-many-locals: Too many local variables (16/15).
[error] 263-263: pylint too-many-locals: Too many local variables (18/15).
[error] 1-1: pylint missing-module-docstring: Missing module docstring.
[error] 4-9: pylint import-error: Unable to import 'pytest' and 'dateutil'.
[error] 13-13: pylint missing-function-docstring: Missing function or method docstring.
[warning] 58-58: pylint redefined-outer-name, reimported, import-outside-toplevel: Redefining and reimporting 'tempfile' outside toplevel.
[error] 59-59: pylint consider-using-with: Consider using 'with' for resource-allocating operations.
[warning] 80-80: pylint redefined-outer-name, reimported, import-outside-toplevel: Redefining and reimporting 'tempfile' outside toplevel.
[error] 81-81: pylint consider-using-with: Consider using 'with' for resource-allocating operations.
[warning] 102-103: pylint redefined-outer-name, reimported, import-outside-toplevel: Redefining and reimporting 'tempfile' outside toplevel; consider using 'with' for resource-allocating operations.
[warning] 160-165: pylint redefined-outer-name, reimported, import-outside-toplevel: Redefining and reimporting 'tempfile' and 'os' outside toplevel; consider using 'with' for resource-allocating operations.
[error] 178-178: pylint consider-using-with: Consider using 'with' for resource-allocating operations.
[warning] 265-265: pylint unspecified-encoding: Using open without explicitly specifying an encoding.
[warning] 351-352: pylint redefined-outer-name, reimported, import-outside-toplevel, unused-variable: Redefining and reimporting 'Path' outside toplevel; unused variable 'meetings'.
[warning] 423-423: pylint redefined-outer-name, reimported, import-outside-toplevel: Redefining and reimporting 'Path' outside toplevel.
[warning] 480-486: pylint redefined-outer-name, reimported, import-outside-toplevel: Redefining and reimporting 'tempfile' and 'os' outside toplevel; consider using 'with' for resource-allocating operations.
[warning] 529-529: pylint import-outside-toplevel: Import outside toplevel (time).
[warning] 766-766: pylint unused-argument: Unused argument 'capsys'.
[warning] 787-787: pylint redefined-outer-name, reimported, import-outside-toplevel: Redefining and reimporting 'datetime' outside toplevel.
[warning] 5-8: pylint wrong-import-order: Standard imports should be placed before third party imports (os, datetime.datetime, unittest.mock.patch, pathlib.Path).
🔇 Additional comments (3)
pyproject.toml (2)
29-29: LGTM: Bandit dependency added correctly.The addition of
bandit>=1.8.5as a development dependency is appropriate for security scanning. The version constraint ensures a reasonably recent version with good functionality.
32-34: Excellent Bandit configuration for test files.The
[tool.bandit]configuration correctly skips B101 (assert_used) warnings, which is appropriate since assertions are expected and necessary in test files. This aligns well with the GitHub Actions workflow configuration.tests/test_calendar_analyzer.py (1)
57-488: Excellent security improvement with temporary files.The overall change from hardcoded paths like
/tmp/dummy.icsto secure temporary files significantly improves the security posture and test isolation. This aligns perfectly with the PR's security scanning objectives and demonstrates good security practices.
Introduces helper functions for creating temporary files in tests, simplifying test setup and teardown. This change improves code readability and reduces redundancy in the test suite by centralizing temporary file creation logic.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/test_calendar_analyzer.py (2)
114-117: Use helper function for consistency.This manual temporary file creation should use the
create_temp_dummy_filehelper function for consistency with the rest of the codebase.Apply this diff to use the helper function:
- # Create a temporary dummy file path (secure alternative to mktemp) - import tempfile - dummy_file = tempfile.NamedTemporaryFile(suffix=".ics", delete=False) - dummy_path = dummy_file.name - dummy_file.close() + # Create a temporary dummy file path (secure alternative to mktemp) + dummy_path = create_temp_dummy_file()
188-196: Use helper function for consistency.This manual temporary file creation should use the
create_temp_dummy_filehelper function for consistency with the rest of the codebase.Apply this diff to use the helper function:
- # Create a temporary dummy file path that doesn't exist - import tempfile - dummy_file = tempfile.NamedTemporaryFile(suffix=".ics", delete=False) - dummy_path = dummy_file.name - dummy_file.close() - # Remove the file to make it nonexistent (for this test) - import os - os.unlink(dummy_path) + # Create a temporary dummy file path that doesn't exist + dummy_path = create_temp_dummy_file() + # Remove the file to make it nonexistent (for this test) + os.unlink(dummy_path)
🧹 Nitpick comments (3)
tests/test_calendar_analyzer.py (3)
93-93: Remove unused import.The
tempfileimport is unused since the test now uses the helper function.- import tempfile
135-135: Remove unused import.The
tempfileimport is unused since the test now uses the helper function.- import tempfile
497-497: Remove unused import.The
tempfileimport is unused since the test now uses the helper function.- import tempfile
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cspell.json(2 hunks)tests/test_calendar_analyzer.py(14 hunks)
✅ Files skipped from review due to trivial changes (1)
- cspell.json
🧰 Additional context used
🪛 Ruff (0.11.9)
tests/test_calendar_analyzer.py
44-44: Use a context manager for opening files
(SIM115)
93-93: tempfile imported but unused
Remove unused import: tempfile
(F401)
114-114: Use a context manager for opening files
(SIM115)
135-135: tempfile imported but unused
Remove unused import: tempfile
(F401)
190-190: Use a context manager for opening files
(SIM115)
497-497: tempfile imported but unused
Remove unused import: tempfile
(F401)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Analyze (python)
- GitHub Check: test (3.12)
- GitHub Check: test (3.13)
- GitHub Check: test (3.9)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
🔇 Additional comments (2)
tests/test_calendar_analyzer.py (2)
13-30: Helper function implementation looks good.The
create_temp_ics_filefunction properly uses a context manager and handles resource management correctly. This addresses the previous review comments about secure temporary file creation.
68-88: Excellent refactoring of test setup.The conversion to use the
create_temp_ics_filehelper function greatly improves code maintainability and addresses the previous concerns about resource management and code duplication.
Adds docstrings to the tests. Addresses potential issues and improves robustness.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Pull Request Overview
This PR integrates Bandit security scanning into the project, updates test files to use secure temporary file helpers, and documents security setup in the README.
- Added Bandit as a dev dependency and configured it to skip B101 in tests
- Refactored tests to use
create_temp_ics_fileandcreate_temp_dummy_filehelpers - Updated README and GitHub Actions workflow to include Bandit instructions
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_calendar_analyzer.py | Refactored to use secure temp file helpers and added os.unlink calls in some tests |
| pyproject.toml | Added Bandit dev dependency and [tool.bandit] config |
| cspell.json | Added new words to spelling dictionary |
| README.md | Added "Security Scanning" section with Bandit usage |
| .github/workflows/bandit.yml | Enabled skipping B101 warnings in CI Bandit scans |
Comments suppressed due to low confidence (1)
tests/test_calendar_analyzer.py:287
- The indentation of this
withblock has changed and may misalign the nested statements, leading to anIndentationError. Ensure the block and its body are consistently indented.
with open(output_path, 'r', encoding='utf-8') as f:
| with tempfile.NamedTemporaryFile(suffix=".ics", mode="w+", delete=False) as tmp: | ||
| tmp.write(ics_content) | ||
| tmp.flush() | ||
| tmp_path = create_temp_ics_file(ics_content) |
There was a problem hiding this comment.
This test creates a temporary file but doesn't remove it. Add os.unlink(tmp_path) at the end to clean up and avoid leaking files.
| with tempfile.NamedTemporaryFile(suffix=".ics", mode="w+", delete=False) as tmp: | ||
| tmp.write(ics_content) | ||
| tmp.flush() | ||
| tmp_path = create_temp_ics_file(ics_content) |
There was a problem hiding this comment.
Temporary files created here are not cleaned up after the test. Consider adding os.unlink(tmp_path) at the end of the test to ensure proper cleanup.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
tests/test_calendar_analyzer.py (3)
32-35: Unnecessaryflush()inside awithblock
tmp.flush()is redundant; the file is automatically flushed and closed on exit from the context manager.- tmp.write(content) - tmp.flush() - return tmp.name + tmp.write(content) + return tmp.name
74-94: Temporary artefacts leak into the test runner’s/tmp
test_analyze_mock_ics(and a few similar tests) create an ICS file viacreate_temp_ics_filebut never delete it, while other tests remember toos.unlink(...). The suite therefore leaves stray files behind, which can eventually fill the tmpfs on long-running CI agents.Add a
try/finallyor usetmp_path_factory/tmp_pathfixtures to guarantee cleanup.Also applies to: 295-298
50-52:create_temp_dummy_filestill leaves an empty file on diskIf the intent is to obtain a non-existent path, delete the file inside the helper:
with tempfile.NamedTemporaryFile(suffix=suffix, delete=False) as dummy_file: dummy_path = dummy_file.name - return dummy_path + os.unlink(dummy_path) + return dummy_pathThat removes the need for ad-hoc
os.unlink()calls scattered through the tests.README.md (1)
198-205: Minor grammar / punctuation issues in new “Security Scanning” sectionConsider:
“Run a security scan on all Python files” and add a comma after “warnings”.Not blocking, just polishing.
cspell.json (2)
5-6: Questionable inclusion of highly project-specific term
"abirismyname"looks like a one-off handle rather than a domain term. Carrying personal / arbitrary nicknames in the global word list pollutes the dictionary for every contributor.If the word is only needed in a single test or comment, consider:
- Removing it from
cspell.json, and- Instead adding an inline
cspell:disable-lineor a local.cspell.jsonin that component’s directory.This keeps the shared dictionary focused and maintainable.
24-27: Verify necessity of “SARIF” vs. “shundor” additions
•SARIFis security-related and makes sense. ✅
•shundorappears non-English/slang; confirm it’s used in code or docs. If it’s only in a commit message or personal comment, omit it for the same cleanliness reasons as above.While touching this section, you may also want to keep the list alphabetically sorted—helps avoid accidental duplicates and eases future diffs (optional).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.github/workflows/bandit.yml(1 hunks)README.md(1 hunks)cspell.json(2 hunks)pyproject.toml(1 hunks)tests/test_calendar_analyzer.py(15 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/bandit.yml
[error] 48-48: trailing spaces
(trailing-spaces)
🪛 LanguageTool
README.md
[uncategorized] ~198-~198: You might be missing the article “a” here.
Context: ...it -r tests/ --skip B101 4. Run security scan on all Python files: bash ...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~204-~204: Possible missing comma found.
Context: ...ps B101 (assert_used) warnings for test files since assertions are expected and appro...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (1)
pyproject.toml (1)
29-34: Confirm Bandit Version & Move Skip Configuration
I wasn’t able to fetch the latest Bandit release due to SSL errors—please confirm the current PyPI version and update the constraint accordingly. Note that Bandit ≤1.7.x does not support[tool.bandit]in pyproject.toml, so your skip list will be ignored.
- File: pyproject.toml
Lines: 29–34- Proposed diff:
- "bandit>=1.8.5", + "bandit>=1.7.7", # or the latest confirmed version- Move the skip list into a .bandit file or invoke Bandit with
--skip B101in your CI/local scripts to ensure B101 is actually skipped.
.github/workflows/bandit.yml
Outdated
| # comma-separated list of test IDs to skip | ||
| skips: B101 # skip assert_used warnings in test files | ||
| # path to a .bandit file that supplies command line arguments |
There was a problem hiding this comment.
🛠️ Refactor suggestion
YAML-lint failure: trailing whitespace & blanket B101 skip
- Line 48 contains trailing spaces – flagged by yamllint.
- Skipping
B101for the whole repo hidesassertmisuse in production code. Prefer two jobs:
• scantests/with--skip B101
• scan everything else without the skip.
- # comma-separated list of test IDs to skip
- skips: B101 # skip assert_used warnings in test files
+ # comma-separated list of test IDs to skip
+ skips: B101📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # comma-separated list of test IDs to skip | |
| skips: B101 # skip assert_used warnings in test files | |
| # path to a .bandit file that supplies command line arguments | |
| # comma-separated list of test IDs to skip | |
| skips: B101 | |
| # path to a .bandit file that supplies command line arguments |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 48-48: trailing spaces
(trailing-spaces)
🤖 Prompt for AI Agents
In .github/workflows/bandit.yml around lines 48 to 50, remove the trailing
whitespace on line 48 to fix the yamllint error. Instead of skipping B101
globally, split the Bandit scan into two separate jobs: one scanning the tests/
directory with the B101 skip enabled, and another scanning the rest of the
codebase without skipping B101, to ensure assert misuse is detected in
production code.
Implements Bandit security scanning to identify potential vulnerabilities.
Includes Bandit as a development dependency and configures it to skip B101 (assert_used) warnings in test files where assertions are expected.
Updates the README with instructions on how to run Bandit locally, and adds a description of basic security features.
Also improves test file handling by using secure temporary file creation.
Summary by CodeRabbit