Add unit tests for core protocol libraries and SOCKS proxy (#1384, #1385)#1451
Open
get-D wants to merge 3 commits intoOWASP:masterfrom
Open
Add unit tests for core protocol libraries and SOCKS proxy (#1384, #1385)#1451get-D wants to merge 3 commits intoOWASP:masterfrom
get-D wants to merge 3 commits intoOWASP:masterfrom
Conversation
Contributor
Summary by CodeRabbit
WalkthroughAdds multiple pytest test modules covering core libraries: Base, FTP/FTPS, POP3/POP3S, SMTP/SMTPS, SSH, Telnet, and socks_proxy. Tests use mocks to validate brute-force/client flows, error propagation, content-filtering behavior, and engine↔library wiring. No production code changed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 3
🧹 Nitpick comments (1)
tests/core/test_socks_proxy.py (1)
75-102: Add return value assertions for completeness.Both SOCKS4 tests only verify that
set_default_proxyis called correctly but don't assert the return values (sockandaddr_info). For consistency with the SOCKS5 tests and to ensure the function returns the expected socket/getaddrinfo pair, add these assertions.Proposed fix
class TestSetSocksProxySocks4: def test_socks4_no_auth(self): mock_socks = MagicMock() mock_socks.SOCKS5 = 2 mock_socks.SOCKS4 = 1 with patch.dict(sys.modules, {"socks": mock_socks}): - set_socks_proxy("socks4://10.0.0.1:1080") + sock, addr_info = set_socks_proxy("socks4://10.0.0.1:1080") mock_socks.set_default_proxy.assert_called_once_with( 1, # SOCKS4 "10.0.0.1", 1080, ) + assert sock is mock_socks.socksocket + assert addr_info is getaddrinfo def test_no_scheme_defaults_to_socks4(self): mock_socks = MagicMock() mock_socks.SOCKS5 = 2 mock_socks.SOCKS4 = 1 with patch.dict(sys.modules, {"socks": mock_socks}): - set_socks_proxy("192.168.1.1:1080") + sock, addr_info = set_socks_proxy("192.168.1.1:1080") mock_socks.set_default_proxy.assert_called_once_with( 1, # SOCKS4 "192.168.1.1", 1080, ) + assert sock is mock_socks.socksocket + assert addr_info is getaddrinfo🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/test_socks_proxy.py` around lines 75 - 102, The SOCKS4 tests call set_socks_proxy but don't assert its return values; update TestSetSocksProxySocks4.test_socks4_no_auth and test_no_scheme_defaults_to_socks4 to capture the tuple returned by set_socks_proxy (e.g., sock, addr_info) and add assertions that sock is the mocked socket object and addr_info matches the expected getaddrinfo result (similar to the SOCKS5 tests), using the existing mock_socks to supply the socket and getaddrinfo behavior so the tests verify both side effects and return values from set_socks_proxy.
🤖 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_ftp.py`:
- Line 43: Append an explicit lint suppression to the intentional failing FTP
call by adding a trailing "# noqa: S321" to the mock setup line
(mock_conn.login.side_effect = ftplib.error_perm("530 Login incorrect")) in
tests/core/lib/test_ftp.py so the test-only FTP security lint hit is ignored.
In `@tests/core/lib/test_ssh.py`:
- Line 12: The hardcoded credential constant PASSWORD triggers Ruff S105 in
tests; suppress the warning by adding a ruff noqa for S105 to that declaration.
Update the PASSWORD assignment (symbol: PASSWORD) to include an inline
suppression comment like "# noqa: S105" (or add "ruff: noqa: S105" at file top)
so the test keeps the literal but CI linting ignores S105.
In `@tests/core/test_socks_proxy.py`:
- Around line 57-72: In test_socks5_with_auth, addr_info is currently unpacked
but never used and the test lacks the same addr_info assertion present in
test_socks5_no_auth; after calling set_socks_proxy in test_socks5_with_auth, add
an assertion that addr_info is getaddrinfo (i.e. assert addr_info is
getaddrinfo) to both use the variable and mirror the other test, leaving the
hardcoded test password as-is.
---
Nitpick comments:
In `@tests/core/test_socks_proxy.py`:
- Around line 75-102: The SOCKS4 tests call set_socks_proxy but don't assert its
return values; update TestSetSocksProxySocks4.test_socks4_no_auth and
test_no_scheme_defaults_to_socks4 to capture the tuple returned by
set_socks_proxy (e.g., sock, addr_info) and add assertions that sock is the
mocked socket object and addr_info matches the expected getaddrinfo result
(similar to the SOCKS5 tests), using the existing mock_socks to supply the
socket and getaddrinfo behavior so the tests verify both side effects and return
values from set_socks_proxy.
🪄 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: 59df6d74-4620-4bcd-8260-7d9a37a399ae
📒 Files selected for processing (7)
tests/core/lib/test_base.pytests/core/lib/test_ftp.pytests/core/lib/test_pop3.pytests/core/lib/test_smtp.pytests/core/lib/test_ssh.pytests/core/lib/test_telnet.pytests/core/test_socks_proxy.py
Signed-off-by: get-D <divyatwa5@gmail.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/core/test_socks_proxy.py (1)
73-100: Add return value assertions to SOCKS4 tests for completeness.Both SOCKS4 tests call
set_socks_proxybut discard the return value. Unlike the SOCKS5 tests which properly assert bothsockandaddr_info, these tests only verify thatset_default_proxywas called correctly but don't validate that the function returns(socks.socksocket, getaddrinfo).Since production code (see
nettacker/core/app.py) relies on unpacking this 2-tuple, validating the return value is important.Proposed fix
def test_socks4_no_auth(self): mock_socks = MagicMock() mock_socks.SOCKS5 = 2 mock_socks.SOCKS4 = 1 with patch.dict(sys.modules, {"socks": mock_socks}): - set_socks_proxy("socks4://10.0.0.1:1080") + sock, addr_info = set_socks_proxy("socks4://10.0.0.1:1080") mock_socks.set_default_proxy.assert_called_once_with( 1, # SOCKS4 "10.0.0.1", 1080, ) + assert sock is mock_socks.socksocket + assert addr_info is getaddrinfo def test_no_scheme_defaults_to_socks4(self): mock_socks = MagicMock() mock_socks.SOCKS5 = 2 mock_socks.SOCKS4 = 1 with patch.dict(sys.modules, {"socks": mock_socks}): - set_socks_proxy("192.168.1.1:1080") + sock, addr_info = set_socks_proxy("192.168.1.1:1080") mock_socks.set_default_proxy.assert_called_once_with( 1, # SOCKS4 "192.168.1.1", 1080, ) + assert sock is mock_socks.socksocket + assert addr_info is getaddrinfo🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/test_socks_proxy.py` around lines 73 - 100, The SOCKS4 tests call set_socks_proxy but never assert its return; update both TestSetSocksProxySocks4.test_socks4_no_auth and test_no_scheme_defaults_to_socks4 to capture the return value from set_socks_proxy (e.g., sock, addr_info = set_socks_proxy(...)) and add assertions that the first element is the mocked socks socket (mock_socks.socksocket) and the second is the address-resolution callable used by the code (addr_info is getaddrinfo or callable), while keeping the existing mock_socks.set_default_proxy assertions intact so the tests validate the full (socket, getaddrinfo) 2-tuple contract from set_socks_proxy.
🤖 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/test_socks_proxy.py`:
- Around line 73-100: The SOCKS4 tests call set_socks_proxy but never assert its
return; update both TestSetSocksProxySocks4.test_socks4_no_auth and
test_no_scheme_defaults_to_socks4 to capture the return value from
set_socks_proxy (e.g., sock, addr_info = set_socks_proxy(...)) and add
assertions that the first element is the mocked socks socket
(mock_socks.socksocket) and the second is the address-resolution callable used
by the code (addr_info is getaddrinfo or callable), while keeping the existing
mock_socks.set_default_proxy assertions intact so the tests validate the full
(socket, getaddrinfo) 2-tuple contract from set_socks_proxy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5835824d-7280-4dd4-8017-180e773a491f
📒 Files selected for processing (1)
tests/core/test_socks_proxy.py
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/core/lib/test_ftp.py (1)
43-43:⚠️ Potential issue | 🟡 MinorAdd explicit Ruff suppression for intentional FTP error-path mock.
Line 43 still triggers S321; mark this as intentional test-only FTP usage to keep lint green.
🔧 Minimal fix
- mock_conn.login.side_effect = ftplib.error_perm("530 Login incorrect") + mock_conn.login.side_effect = ftplib.error_perm( # noqa: S321 - intentional FTP test path + "530 Login incorrect" + )As per coding guidelines "Line length: maximum 99 characters (enforced by
ruff,ruff-format, andisortwith black profile)" and this unresolved Ruff finding currently blocks that standard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/lib/test_ftp.py` at line 43, The test intentionally sets mock_conn.login.side_effect = ftplib.error_perm("530 Login incorrect") to exercise an FTP error path, but ruff flags S321; add an inline ruff suppression on that statement (e.g., append a noqa for S321) so the linter knows this use is intentional for tests and keep the rest of the test unchanged; target the mock_conn.login.side_effect line when applying the suppression.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/core/lib/test_ftp.py`:
- Line 43: The test intentionally sets mock_conn.login.side_effect =
ftplib.error_perm("530 Login incorrect") to exercise an FTP error path, but ruff
flags S321; add an inline ruff suppression on that statement (e.g., append a
noqa for S321) so the linter knows this use is intentional for tests and keep
the rest of the test unchanged; target the mock_conn.login.side_effect line when
applying the suppression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a413b84b-dd30-424f-aa63-1010ccff831d
📒 Files selected for processing (7)
tests/core/lib/test_ftp.pytests/core/lib/test_pop3.pytests/core/lib/test_smb.pytests/core/lib/test_smtp.pytests/core/lib/test_ssh.pytests/core/lib/test_telnet.pytests/core/test_socks_proxy.py
✅ Files skipped from review due to trivial changes (2)
- tests/core/lib/test_smb.py
- tests/core/lib/test_telnet.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/core/lib/test_ssh.py
- tests/core/lib/test_smtp.py
- tests/core/lib/test_pop3.py
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.
Description
This PR adds a comprehensive set of unit tests for the core protocol libraries and the SOCKS proxy module, significantly improving test coverage in the core engine.
All tests are designed to run fully offline by using Python’s
unittest.mockto simulate network interactions. This ensures consistent and reliable test results without requiring external connections.This work addresses Issue #1384 (unit tests for core protocol libraries) and Issue #1385 (unit tests for the SOCKS proxy). It also contributes toward the GSoC 2026 goal of achieving ~85% test coverage.
What’s Included
This PR introduces 7 new test files with a total of 55 unit tests, all passing successfully:
test_socks_proxy.py (10 tests)
Covers different
getaddrinfoformats andset_socks_proxybehavior, including SOCKS4/5 configurations (with and without authentication) and fallback scenarios. Uses mocking to safely handle local imports.test_ftp.py (9 tests)
Tests FTP and FTPS libraries, including successful logins, authentication failures, connection resets, and TLS handling.
test_pop3.py (9 tests)
Covers POP3/POP3S behavior, including authentication errors, dropped connections, and SSL-related logic.
test_smtp.py (8 tests)
Validates SMTP/SMTPS flows, especially ensuring
starttls()is correctly executed before authentication.test_ssh.py (6 tests)
Tests authentication strategy selection (password vs NoneAuth) and verifies consistent use of
AutoAddPolicy.test_base.py (9 tests)
Covers default behaviors in
BaseLibraryand validatesfilter_large_content()logic, including proper truncation at word boundaries.test_telnet.py (4 tests)
Tests Telnet connections, including success cases, timeouts, and connection drops.
Test Results
All tests pass successfully on supported Python versions.
Tested locally using Poetry with Python 3.12
(Note: telnet-related tests work on Python 3.12, before
telnetlibis removed in Python 3.13.)Impact
Fixes #1384
Fixes #1385