Open
Conversation
- Tests weak algorithms: sha1, md5, md2, md4 - Tests case insensitivity (uppercase input) - Tests safe algorithms: sha256, sha512, sha384 - Tests edge cases: empty string, random string - All 11 tests passing Part of improving test coverage for GSoC 2026
- TestIsSingleIPv4: 12 tests for IPv4 address validation - TestIsSingleIPv6: 10 tests including None bug documentation - TestIsIPv4Range: 8 tests (documents naming swap with is_ipv4_cidr) - TestIsIPv4CIDR: 7 tests (documents naming swap with is_ipv4_range) - TestIsIPv6Range: 6 tests for IPv6 dash-range detection - TestIsIPv6CIDR: 8 tests for IPv6 CIDR detection - TestGenerateIPRange: 7 tests covering both code branches - TestGetIPRange: 4 tests using unittest.mock for HTTP isolation Coverage: nettacker/core/ip.py 0% -> 83% Note: is_ipv4_range/is_ipv4_cidr and is_ipv6_range/is_ipv6_cidr appear to have swapped names - documented in test docstrings
- TestFtpLibraryBruteForce: 8 tests covering successful login, method call verification, error_perm on wrong password, timeout handling, and return value structure - TestFtpEngine: 2 tests for engine instantiation and library assignment - All network calls mocked via patch.object(FtpLibrary, 'client') Coverage: nettacker/core/lib/ftp.py 0% -> 100%
Contributor
Summary by CodeRabbit
WalkthroughAdded comprehensive unit tests for FTP and FTPS functionality alongside an error-handling improvement. Tests validate FTP/FTPS engine initialization, brute-force method behavior, exception propagation, and timeout handling. Production code now wraps connection operations in try-finally to ensure proper resource cleanup. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/core/lib/test_ftp.py (1)
65-75: Minor redundancy withtest_successful_login_returns_dict.This test checks that keys exist in the result, but
test_successful_login_returns_dictalready implicitly verifies this by asserting specific values for the same keys. Consider consolidating or removing this test to reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/lib/test_ftp.py` around lines 65 - 75, The test_result_contains_all_keys test is redundant because test_successful_login_returns_dict already asserts the same keys/values; remove test_result_contains_all_keys (or merge its intent into test_successful_login_returns_dict) to avoid duplicate assertions—locate the test function named test_result_contains_all_keys in tests/core/lib/test_ftp.py that patches FtpLibrary.client and either delete that test or replace it with a focused assertion (e.g., only validate structure differences not covered by test_successful_login_returns_dict) so the suite remains concise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/core/lib/test_ssl.py`:
- Around line 19-38: Add a test that verifies non-prefix matches for the current
"unsafe_algo in algo" behavior of is_weak_hash_algo by asserting that an
algorithm string with the weak token not at the start (e.g.,
"rsaWithSHA1Encryption") returns True; locate the test cases in
tests/core/lib/test_ssl.py alongside the other weak-hash tests and add a new
test method (mirroring naming style like test_sha1_uppercase_is_weak) that calls
is_weak_hash_algo("rsaWithSHA1Encryption") and asserts True so the suite will
detect changes from containment to startswith logic.
In `@tests/test_ip.py`:
- Around line 261-296: Add a success-path unit test (e.g.,
test_success_returns_generated_range) that mocks nettacker.core.ip.requests.get
to return a valid API payload (set return_value.content to JSON bytes matching
the structure get_ip_range expects), call get_ip_range with a sample IP, and
assert the result is the non-fallback range by comparing it to the output of
generate_ip_range for that sample IP (i.e., result ==
generate_ip_range(sample_ip) and result != [sample_ip]); use the same patch
style as the existing tests.
- Line 1: Move the test file so it mirrors the package layout: create
tests/core/ and move tests/test_ip.py to tests/core/test_ip.py; then update the
test's import(s) to import the production module using its package path (e.g.,
from nettacker.core.ip import <function_or_class_under_test> or import
nettacker.core.ip as ip) so pytest discovers it under the mirrored layout and
the tests reference the correct module symbols.
- Around line 93-97: The test currently catches any Exception using
pytest.raises((TypeError, Exception)); narrow this to the specific expected
exception by replacing the tuple with TypeError only so test_none_raises asserts
that is_single_ipv6(None) raises a TypeError (the error coming from
netaddr.valid_ipv6(None)), updating the pytest.raises usage in the
test_none_raises function accordingly.
---
Nitpick comments:
In `@tests/core/lib/test_ftp.py`:
- Around line 65-75: The test_result_contains_all_keys test is redundant because
test_successful_login_returns_dict already asserts the same keys/values; remove
test_result_contains_all_keys (or merge its intent into
test_successful_login_returns_dict) to avoid duplicate assertions—locate the
test function named test_result_contains_all_keys in tests/core/lib/test_ftp.py
that patches FtpLibrary.client and either delete that test or replace it with a
focused assertion (e.g., only validate structure differences not covered by
test_successful_login_returns_dict) so the suite remains concise.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 008303d2-a33b-4a14-a10f-73a78e9185a6
📒 Files selected for processing (4)
.github/workflows/ci_cd.ymltests/core/lib/test_ftp.pytests/core/lib/test_ssl.pytests/test_ip.py
pUrGe12
suggested changes
Apr 2, 2026
Raavi29
added a commit
to Raavi29/Nettacker
that referenced
this pull request
Apr 2, 2026
Addresses CodeRabbit feedback on PR OWASP#1487 - all previous weak hash tests had the weak token at the start of the string. This test confirms is_weak_hash_algo uses 'in' not 'startswith', so weak tokens are detected anywhere in the algorithm string.
Author
|
Latest commits: removed tests/test_ip.py (duplicate of tests/core/test_ip.py), restored original 574-line SSL test suite that was accidentally stripped from this branch. |
arkid15r
changed the title
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/core/lib/test_ftp.py (1)
43-54: Minor: unused mock unpacking.Line 46 unpacks the fixture but discards both values. Since the fixture is needed to activate the patch but neither mock is used in this test, you could simplify by removing the unpacking.
💅 Optional simplification
def test_successful_login_returns_dict( - self, ftp_client_mocks, host, port, username, password, timeout + self, ftp_client_mocks, host, port, username, password, timeout # noqa: ARG002 ): - _, _ = ftp_client_mocks lib = FtpLibrary() result = lib.brute_force(host, port, username, password, timeout)Alternatively, if linter complains about unused argument, the
# noqacomment or a leading underscore (_ftp_client_mocks) signals intentional non-use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/lib/test_ftp.py` around lines 43 - 54, The test test_successful_login_returns_dict unnecessarily unpacks the ftp_client_mocks fixture into two discarded values; remove the "_, _ = ftp_client_mocks" unpacking (or rename the parameter to _ftp_client_mocks or add a "# noqa" comment) so the fixture still activates the patch without introducing unused variables—update the test_successful_login_returns_dict function accordingly.tests/core/lib/test_ftps.py (1)
23-31: Consider verifyingclose()is called for consistency.The FTP tests verify
close()is called afterbrute_force(). For consistency and completeness, consider adding that assertion here too—even thoughFtpsLibraryinherits fromFtpLibrary, explicit verification documents the expected behavior for FTPS clients.💡 Optional addition
`@patch.object`(FtpsLibrary, "client") def test_brute_force_uses_ftp_tls_client(self, mock_ftp_tls_class): mock_connection = MagicMock(spec=ftplib.FTP_TLS) mock_ftp_tls_class.return_value = mock_connection lib = FtpsLibrary() lib.brute_force("10.0.0.1", 21, "user", "pass", timeout=30) mock_ftp_tls_class.assert_called_once_with(timeout=30) + mock_connection.close.assert_called_once()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/lib/test_ftps.py` around lines 23 - 31, The test test_brute_force_uses_ftp_tls_client should also assert that the FTPS client is closed; after calling FtpsLibrary().brute_force(...) add an assertion on the mocked FTPS connection (mock_connection) that close() was called (e.g., mock_connection.close.assert_called_once()) to mirror the FTP test and document expected cleanup behavior for FtpsLibrary.client and brute_force.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/core/lib/test_ftp.py`:
- Around line 43-54: The test test_successful_login_returns_dict unnecessarily
unpacks the ftp_client_mocks fixture into two discarded values; remove the "_, _
= ftp_client_mocks" unpacking (or rename the parameter to _ftp_client_mocks or
add a "# noqa" comment) so the fixture still activates the patch without
introducing unused variables—update the test_successful_login_returns_dict
function accordingly.
In `@tests/core/lib/test_ftps.py`:
- Around line 23-31: The test test_brute_force_uses_ftp_tls_client should also
assert that the FTPS client is closed; after calling
FtpsLibrary().brute_force(...) add an assertion on the mocked FTPS connection
(mock_connection) that close() was called (e.g.,
mock_connection.close.assert_called_once()) to mirror the FTP test and document
expected cleanup behavior for FtpsLibrary.client and brute_force.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0551b06a-797f-4f02-9b72-6ba52ab3bfd8
📒 Files selected for processing (3)
nettacker/core/lib/ftp.pytests/core/lib/test_ftp.pytests/core/lib/test_ftps.py
Author
Thank you for the improvements and the feedback. GPG signing is now configured and the latest commit is signed. Apologies for the delay on that. |
There was a problem hiding this comment.
Pull request overview
Adds automated test coverage for the FTP/FTPS libraries and tightens FTP connection cleanup behavior within Nettacker’s core libraries.
Changes:
- Added unit tests for
FtpLibrary/FtpEnginebehavior and error propagation. - Added basic unit tests for
FtpsLibrary/FtpsEngineinheritance and client usage. - Updated
FtpLibrary.brute_forceto always close the FTP connection viatry/finally.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
tests/core/lib/test_ftp.py |
New unit tests covering FtpLibrary.brute_force success paths, error propagation, and timeout forwarding. |
tests/core/lib/test_ftps.py |
New unit tests validating FTPS engine/library wiring and that FTP_TLS client is used. |
nettacker/core/lib/ftp.py |
Ensures connection.close() is executed even when connect()/login() raises. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed change
Summary
Adds unit tests for
nettacker/core/lib/ftp.py, bringing coverage from 0% to 100% (12 statements, 0 missed).Tests Added
TestFtpLibraryBruteForce- 8 tests covering:connect(),login(),close()each called with correct argumentsftplib.error_permraised on wrong credentialsTimeoutErrorraised on connection timeoutTestFtpEngine- 2 tests for engine instantiation and library assignmentApproach
FtpLibraryusesself.client(assigned toftplib.FTP) rather than callingftplib.FTPdirectly. All tests usepatch.object(FtpLibrary, 'client')to intercept network calls - no real FTP connections made.Tests run
python -m pytest tests/core/lib/test_ftp.py -v— 10 passedType of change
Checklist
make pre-commitand confirm it didn't generate any warnings/changesmake test, I confirm all tests passed locallydocs/folder