Add tests for FtpLibrary - 10 tests, 100% coverage of ftp.py#1487
Open
Raavi29 wants to merge 5 commits intoOWASP:masterfrom
Open
Add tests for FtpLibrary - 10 tests, 100% coverage of ftp.py#1487Raavi29 wants to merge 5 commits intoOWASP:masterfrom
Raavi29 wants to merge 5 commits intoOWASP:masterfrom
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
WalkthroughThis PR pins the CodeQL GitHub Action to a new version and adds comprehensive test coverage for FTP library functionality, IP utility parsing, and weak hash algorithm detection, while significantly refactoring existing SSL test coverage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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
Comment on lines
+19
to
+38
| def test_sha1_is_weak(self): | ||
| assert is_weak_hash_algo("sha1WithRSAEncryption") is True | ||
|
|
||
| def version(self): | ||
| return self.Version | ||
| def test_md5_is_weak(self): | ||
| assert is_weak_hash_algo("md5WithRSAEncryption") is True | ||
|
|
||
| def test_md2_is_weak(self): | ||
| assert is_weak_hash_algo("md2WithRSAEncryption") is True | ||
|
|
||
| class SubjectObject: | ||
| def __init__(self, subject="subject"): | ||
| self.subject = subject | ||
| def test_md4_is_weak(self): | ||
| assert is_weak_hash_algo("md4WithRSAEncryption") is True | ||
|
|
||
| def get_components(self): | ||
| return [ | ||
| (b"component", str.encode(self.subject)), | ||
| ] | ||
| # --- Case insensitivity — function lowercases input, so these must also work --- | ||
|
|
||
| def test_sha1_uppercase_is_weak(self): | ||
| # The function does algo.lower() so uppercase should still be caught | ||
| assert is_weak_hash_algo("SHA1WithRSAEncryption") is True | ||
|
|
||
| class IssuerObject: | ||
| def __init__(self, issuer="issuer"): | ||
| self.issuer = issuer | ||
| def test_md5_uppercase_is_weak(self): | ||
| assert is_weak_hash_algo("MD5WithRSAEncryption") is True |
Contributor
There was a problem hiding this comment.
Add one non-prefix weak-hash case.
nettacker/core/lib/ssl.py:13-18 uses unsafe_algo in algo, not prefix matching. Since every positive input here starts with the weak token, a future change to startswith() would still pass this suite while changing runtime behavior. A case like rsaWithSHA1Encryption would pin the current semantics.
💡 Suggested addition
def test_md5_uppercase_is_weak(self):
assert is_weak_hash_algo("MD5WithRSAEncryption") is True
+
+ def test_sha1_in_middle_is_weak(self):
+ assert is_weak_hash_algo("rsaWithSHA1Encryption") is True📝 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.
Suggested change
| def test_sha1_is_weak(self): | |
| assert is_weak_hash_algo("sha1WithRSAEncryption") is True | |
| def version(self): | |
| return self.Version | |
| def test_md5_is_weak(self): | |
| assert is_weak_hash_algo("md5WithRSAEncryption") is True | |
| def test_md2_is_weak(self): | |
| assert is_weak_hash_algo("md2WithRSAEncryption") is True | |
| class SubjectObject: | |
| def __init__(self, subject="subject"): | |
| self.subject = subject | |
| def test_md4_is_weak(self): | |
| assert is_weak_hash_algo("md4WithRSAEncryption") is True | |
| def get_components(self): | |
| return [ | |
| (b"component", str.encode(self.subject)), | |
| ] | |
| # --- Case insensitivity — function lowercases input, so these must also work --- | |
| def test_sha1_uppercase_is_weak(self): | |
| # The function does algo.lower() so uppercase should still be caught | |
| assert is_weak_hash_algo("SHA1WithRSAEncryption") is True | |
| class IssuerObject: | |
| def __init__(self, issuer="issuer"): | |
| self.issuer = issuer | |
| def test_md5_uppercase_is_weak(self): | |
| assert is_weak_hash_algo("MD5WithRSAEncryption") is True | |
| def test_sha1_is_weak(self): | |
| assert is_weak_hash_algo("sha1WithRSAEncryption") is True | |
| def test_md5_is_weak(self): | |
| assert is_weak_hash_algo("md5WithRSAEncryption") is True | |
| def test_md2_is_weak(self): | |
| assert is_weak_hash_algo("md2WithRSAEncryption") is True | |
| def test_md4_is_weak(self): | |
| assert is_weak_hash_algo("md4WithRSAEncryption") is True | |
| # --- Case insensitivity — function lowercases input, so these must also work --- | |
| def test_sha1_uppercase_is_weak(self): | |
| # The function does algo.lower() so uppercase should still be caught | |
| assert is_weak_hash_algo("SHA1WithRSAEncryption") is True | |
| def test_md5_uppercase_is_weak(self): | |
| assert is_weak_hash_algo("MD5WithRSAEncryption") is True | |
| def test_sha1_in_middle_is_weak(self): | |
| assert is_weak_hash_algo("rsaWithSHA1Encryption") is True |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/lib/test_ssl.py` around lines 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.
| @@ -0,0 +1,296 @@ | |||
| import pytest | |||
Contributor
There was a problem hiding this comment.
Move this test file to mirror the package layout.
nettacker/core/ip.py should be tested from a mirrored path (for example tests/core/test_ip.py), not tests/test_ip.py.
As per coding guidelines, tests/**/*.py: Tests must be located in tests/ directory and mirror the package layout with pattern tests/core/, tests/lib/, etc.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_ip.py` at 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.
Comment on lines
+93
to
+97
| def test_none_raises(self): | ||
| # No str() wrapper in source — None causes TypeError | ||
| # We document this bug with pytest.raises | ||
| with pytest.raises((TypeError, Exception)): | ||
| is_single_ipv6(None) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify where broad raises are used and inspect target implementation.
rg -n --type=py -C2 'pytest\.raises\(\(TypeError,\s*Exception\)\)'
fd 'ip.py$' | xargs -I{} rg -n -C4 'def is_single_ipv6|valid_ipv6' {}Repository: OWASP/Nettacker
Length of output: 1119
Narrow pytest.raises to a specific exception type.
pytest.raises((TypeError, Exception)) is ineffective—it's equivalent to catching Exception, which can mask unrelated failures. The test documentation explicitly notes that only TypeError is expected from netaddr.valid_ipv6(None).
Proposed fix
- with pytest.raises((TypeError, Exception)):
+ with pytest.raises(TypeError):
is_single_ipv6(None)📝 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.
Suggested change
| def test_none_raises(self): | |
| # No str() wrapper in source — None causes TypeError | |
| # We document this bug with pytest.raises | |
| with pytest.raises((TypeError, Exception)): | |
| is_single_ipv6(None) | |
| def test_none_raises(self): | |
| # No str() wrapper in source — None causes TypeError | |
| # We document this bug with pytest.raises | |
| with pytest.raises(TypeError): | |
| is_single_ipv6(None) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_ip.py` around lines 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.
Comment on lines
+261
to
+296
| """ | ||
| get_ip_range() makes a live HTTP request to RIPE's API. | ||
| We mock requests.get to avoid network calls in tests. | ||
| Two behaviours to test: | ||
| 1. Successful API response — returns generate_ip_range() result | ||
| 2. Any failure (network down, bad JSON, missing key) — returns [ip] | ||
| """ | ||
|
|
||
| def test_exception_returns_ip_as_list(self): | ||
| # When anything goes wrong, function returns [ip] as fallback | ||
| # We simulate failure by making requests.get raise an exception | ||
| with patch("nettacker.core.ip.requests.get") as mock_get: | ||
| mock_get.side_effect = Exception("network error") | ||
| result = get_ip_range("8.8.8.8") | ||
| assert result == ["8.8.8.8"] | ||
|
|
||
| def test_invalid_json_returns_ip_as_list(self): | ||
| # Bad JSON response — json.loads will fail, fallback kicks in | ||
| with patch("nettacker.core.ip.requests.get") as mock_get: | ||
| mock_get.return_value.content = b"not valid json" | ||
| result = get_ip_range("1.1.1.1") | ||
| assert result == ["1.1.1.1"] | ||
|
|
||
| def test_missing_key_returns_ip_as_list(self): | ||
| # Valid JSON but wrong structure — KeyError triggers fallback | ||
| with patch("nettacker.core.ip.requests.get") as mock_get: | ||
| mock_get.return_value.content = b'{"unexpected": "structure"}' | ||
| result = get_ip_range("9.9.9.9") | ||
| assert result == ["9.9.9.9"] | ||
|
|
||
| def test_returns_list_type(self): | ||
| # Whatever happens, result must always be a list | ||
| with patch("nettacker.core.ip.requests.get") as mock_get: | ||
| mock_get.side_effect = Exception("timeout") | ||
| result = get_ip_range("8.8.8.8") | ||
| assert isinstance(result, list) No newline at end of file |
Contributor
There was a problem hiding this comment.
Add a success-path test for get_ip_range.
This block validates only fallback/error behavior. Please add one test that mocks a valid API payload and asserts the non-fallback returned range, so the primary behavior is guarded too.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_ip.py` around lines 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.
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
1 participant
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