Add TestGetCertInfo tests for get_cert_info() in ssl.py#1489
Open
Raavi29 wants to merge 21 commits intoOWASP:masterfrom
Open
Add TestGetCertInfo tests for get_cert_info() in ssl.py#1489Raavi29 wants to merge 21 commits intoOWASP:masterfrom
Raavi29 wants to merge 21 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
- 13 tests using a real self-signed certificate (sha256, valid 2026-2027) - Tests cover: return type, required keys, self_signed detection, expiry status, subject/issuer content, date format validation, weak_signing_algo with real and mocked is_weak_hash_algo calls - is_weak_hash_algo mocked in 2 tests to verify get_cert_info passes the result through correctly, independent of hash logic - Original 574-line SSL test suite fully preserved Coverage: nettacker/core/lib/ssl.py increases beyond current 15%
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdded module-scoped pytest fixtures and a new TestGetCertInfo test class in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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: 1
🧹 Nitpick comments (2)
tests/test_ip.py (2)
1-12: Remove unused importMagicMock.
MagicMockis imported but never used in this file.Proposed fix
import pytest -from unittest.mock import patch, MagicMock +from unittest.mock import patch from nettacker.core.ip import (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_ip.py` around lines 1 - 12, The import list in tests/test_ip.py includes an unused symbol MagicMock; remove MagicMock from the from unittest.mock import line so only needed names are imported (e.g., keep patch) and update the import statement that currently references MagicMock to avoid linter/test warnings.
57-58: Add blank lines between class definitions.PEP 8 recommends two blank lines between top-level class definitions. Several class boundaries are missing blank lines (e.g., lines 57-58, 97-98, 164-165, 220-221, 259-260).
Example fix at line 57-58
def test_none_input(self): # str(None) = "None" — not a valid IP, should be False assert is_single_ipv4(None) is False + + class TestIsSingleIPv6:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_ip.py` around lines 57 - 58, Add a blank line between top-level class definitions to satisfy PEP 8: insert a single empty line before each class declaration (e.g., place a blank line between the assertion using is_single_ipv4(None) and the class TestIsSingleIPv6 definition) and do the same at the other top-level class boundaries called out in the review so each class (like TestIsSingleIPv6 and the other test classes) is separated by two blank lines from preceding top-level code.
🤖 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 590-638: The hard-coded SELF_SIGNED_CERT will expire on 2027-04-01
and break test_not_expired; replace the static fixture with a runtime-generated
self-signed certificate (or swap to a long-lived fixture) so dates are relative
to now: add a helper (e.g., make_test_self_signed_cert or
generate_self_signed_cert) that creates a cert valid_from = now - 1 day and
valid_to = now + 1 year, use that helper in tests (test_returns_dict,
test_required_keys_present, test_self_signed_is_true, test_not_expired) instead
of SELF_SIGNED_CERT, and keep assertions using get_cert_info(...) unchanged
(ensure the helper returns PEM string compatible with get_cert_info).
---
Nitpick comments:
In `@tests/test_ip.py`:
- Around line 1-12: The import list in tests/test_ip.py includes an unused
symbol MagicMock; remove MagicMock from the from unittest.mock import line so
only needed names are imported (e.g., keep patch) and update the import
statement that currently references MagicMock to avoid linter/test warnings.
- Around line 57-58: Add a blank line between top-level class definitions to
satisfy PEP 8: insert a single empty line before each class declaration (e.g.,
place a blank line between the assertion using is_single_ipv4(None) and the
class TestIsSingleIPv6 definition) and do the same at the other top-level class
boundaries called out in the review so each class (like TestIsSingleIPv6 and the
other test classes) is separated by two blank lines from preceding top-level
code.
🪄 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: 46a942d7-825c-4d9b-9f15-fb9d3c3d037f
📒 Files selected for processing (2)
tests/core/lib/test_ssl.pytests/test_ip.py
Addresses CodeRabbit feedback - hardcoded cert expired 2027-04-01 which would cause test_not_expired to fail deterministically after that date. Certificate is now generated fresh at test runtime using pyOpenSSL with a 10-year validity window.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_ip.py (1)
275-311: Add one success-path test forget_ip_range.Current tests validate fallback behavior well, but the happy path (valid RIPE JSON → parsed range) is still unverified.
Suggested additional test
class TestGetIPRange: @@ 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) + + def test_success_response_returns_generated_range(self): + with patch("nettacker.core.ip.requests.get") as mock_get: + mock_get.return_value.content = ( + b'{"objects":{"object":[{"primary-key":{"attribute":[{"value":"10.0.0.1-10.0.0.3"}]}}]}}' + ) + result = get_ip_range("8.8.8.8") + assert result == ["10.0.0.1", "10.0.0.2", "10.0.0.3"]Based on learnings, "Enforce test coverage via
--cov=nettackeras configured inpyproject.tomlundertool.pytest.ini_options."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_ip.py` around lines 275 - 311, Add a success-path unit test to TestGetIPRange that mocks nettacker.core.ip.requests.get to return a valid RIPE-style JSON payload in .content (bytes) and asserts get_ip_range("A.B.C.D") returns the parsed range; specifically, construct the mock response JSON so it matches the expected structure used by get_ip_range and compare the result to generate_ip_range("A.B.C.D") (import or reference generate_ip_range from nettacker.core.ip) to validate the happy path.
🤖 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/test_ip.py`:
- Line 1: Test file tests/test_ip.py is in the wrong location; move it to mirror
the package layout (e.g., tests/core/test_ip.py) so it matches
nettacker/core/ip.py, then update any imports inside the test to reflect the new
path (change from importing nettacker.core.ip or relative paths if needed) and
run tests to ensure discovery still works with the new file location.
- Around line 99-103: The test test_none_raises currently uses a broad
pytest.raises((TypeError, Exception)); tighten it to assert the specific
expected exception by using pytest.raises(TypeError) when calling
is_single_ipv6(None) so the test only passes for the intended TypeError and
won’t swallow unrelated exceptions.
---
Nitpick comments:
In `@tests/test_ip.py`:
- Around line 275-311: Add a success-path unit test to TestGetIPRange that mocks
nettacker.core.ip.requests.get to return a valid RIPE-style JSON payload in
.content (bytes) and asserts get_ip_range("A.B.C.D") returns the parsed range;
specifically, construct the mock response JSON so it matches the expected
structure used by get_ip_range and compare the result to
generate_ip_range("A.B.C.D") (import or reference generate_ip_range from
nettacker.core.ip) to validate the happy path.
🪄 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: cdcb06c5-5ff6-4b75-beed-df313ab40ddb
📒 Files selected for processing (2)
tests/core/lib/test_ssl.pytests/test_ip.py
✅ Files skipped from review due to trivial changes (1)
- tests/core/lib/test_ssl.py
Contributor
Author
|
Latest commits: switched from hardcoded certificate to runtime-generated cert using pyOpenSSL with 10-year validity - addresses CodeRabbit feedback about time-coupled fixture. Also removed tests/test_ip.py duplicate. |
There was a problem hiding this comment.
Pull request overview
Adds dedicated unit coverage for get_cert_info() in nettacker/core/lib/ssl.py by extending the existing SSL test suite.
Changes:
- Import
get_cert_infointo the SSL test module. - Add
TestGetCertInfowith runtime-generated self-signed cert coverage for expected fields/behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/core/lib/test_ssl.py (1)
639-649: Strengthen date-format assertions.Checking length and dash positions leaves room for strings like
"abcd-ef-gh"to pass. Parsing withdatetime.strptime(..., "%Y-%m-%d")would verify both format and numeric validity in a single assertion.♻️ Proposed refactor
+ from datetime import datetime + def test_activation_date_format(self, self_signed_cert_pem): result = get_cert_info(self_signed_cert_pem) - assert len(result["activation_date"]) == 10 - assert result["activation_date"][4] == "-" - assert result["activation_date"][7] == "-" + datetime.strptime(result["activation_date"], "%Y-%m-%d") def test_expiration_date_format(self, self_signed_cert_pem): result = get_cert_info(self_signed_cert_pem) - assert len(result["expiration_date"]) == 10 - assert result["expiration_date"][4] == "-" - assert result["expiration_date"][7] == "-" + datetime.strptime(result["expiration_date"], "%Y-%m-%d")🤖 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 639 - 649, Replace the weak string checks in test_activation_date_format and test_expiration_date_format (which call get_cert_info) with a strict parse using datetime.strptime(..., "%Y-%m-%d") to validate both format and numeric date validity; update the tests to call datetime.datetime.strptime(result["activation_date"], "%Y-%m-%d") and datetime.datetime.strptime(result["expiration_date"], "%Y-%m-%d") respectively and add/import datetime at the top of the test file if not already present.
🤖 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`:
- Line 1: The file containing the line "import ssl" has mixed CRLF/LF endings;
normalize the file to use consistent LF line endings (remove any CR characters)
and re-save, then run the pre-commit hooks (e.g., make pre-commit) to verify the
mixed-line-ending check passes and commit the normalized file so CI succeeds.
- Around line 580-600: The fixture self_signed_cert_pem declares scope="module"
but the docstring says the cert is "created fresh for each test run"; update
either the docstring to state the certificate is generated once per test module
and reused, or change the fixture scope to a narrower one (remove scope or use
scope="class"/"function") so it really is recreated per test; locate the fixture
definition named self_signed_cert_pem and apply the chosen change so wording and
behavior match.
---
Nitpick comments:
In `@tests/core/lib/test_ssl.py`:
- Around line 639-649: Replace the weak string checks in
test_activation_date_format and test_expiration_date_format (which call
get_cert_info) with a strict parse using datetime.strptime(..., "%Y-%m-%d") to
validate both format and numeric date validity; update the tests to call
datetime.datetime.strptime(result["activation_date"], "%Y-%m-%d") and
datetime.datetime.strptime(result["expiration_date"], "%Y-%m-%d") respectively
and add/import datetime at the top of the test file if not already present.
🪄 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: f14d83a7-ced3-47c7-857c-771344bd6767
📒 Files selected for processing (1)
tests/core/lib/test_ssl.py
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/core/lib/test_ssl.py (1)
580-585:⚠️ Potential issue | 🟡 MinorDocstring still contradicts the module-scoped fixture, and line 583 exceeds the 99-char limit.
The wording "created fresh for each test run" still conflicts with
scope="module"(the cert is generated once and reused across all tests in the class). Additionally, line 583 is well over 99 characters, which will be flagged byruff/ruff-format.📝 Proposed wording + wrapping fix
- Uses a runtime-generated self-signed certificate so tests never rely on a hardcoded certificate that could expire. Certificate is created fresh for each test run using - pyOpenSSL with a 10-year validity window. + Uses a runtime-generated self-signed certificate so tests never rely on a + hardcoded certificate that could expire. The certificate is generated once + per test module (module-scoped fixture) using pyOpenSSL with a 10-year + validity window.As per coding guidelines: "Line length: maximum 99 characters (enforced by
ruff,ruff-format, andisortwith black profile)".🤖 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 580 - 585, Update the module docstring for the get_cert_info(cert) tests to accurately state that the self-signed certificate is generated once per test module (because a module-scoped fixture is used) instead of "created fresh for each test run", and reflow/wrap the long sentence that describes certificate creation so no line exceeds 99 characters; locate the docstring referencing get_cert_info(cert) and the module-scoped cert fixture and adjust the wording and line breaks accordingly.
🧹 Nitpick comments (1)
tests/core/lib/test_ssl.py (1)
639-649: Prefer a strict parse over length/character checks for date format.
len == 10and[4]/[7] == "-"also accept malformed strings like"ABCD-EF-GH". Adatetime.strptime(..., "%Y-%m-%d")round-trip is a stricter and more readable assertion and aligns with whatget_cert_infoactually produces.♻️ Proposed refactor
+from datetime import datetime + - def test_activation_date_format(self, self_signed_cert_pem): - result = get_cert_info(self_signed_cert_pem) - assert len(result["activation_date"]) == 10 - assert result["activation_date"][4] == "-" - assert result["activation_date"][7] == "-" - - def test_expiration_date_format(self, self_signed_cert_pem): - result = get_cert_info(self_signed_cert_pem) - assert len(result["expiration_date"]) == 10 - assert result["expiration_date"][4] == "-" - assert result["expiration_date"][7] == "-" + def test_activation_date_format(self, self_signed_cert_pem): + result = get_cert_info(self_signed_cert_pem) + datetime.strptime(result["activation_date"], "%Y-%m-%d") + + def test_expiration_date_format(self, self_signed_cert_pem): + result = get_cert_info(self_signed_cert_pem) + datetime.strptime(result["expiration_date"], "%Y-%m-%d")🤖 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 639 - 649, Replace the brittle length-and-character checks in test_activation_date_format and test_expiration_date_format with strict parsing: call get_cert_info(self_signed_cert_pem) as before, then use datetime.strptime(result["activation_date"], "%Y-%m-%d") and datetime.strptime(result["expiration_date"], "%Y-%m-%d") respectively to assert they parse successfully (import datetime at top if missing); keep references to the test functions and get_cert_info to locate and update the assertions.
🤖 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_ssl.py`:
- Around line 580-585: Update the module docstring for the get_cert_info(cert)
tests to accurately state that the self-signed certificate is generated once per
test module (because a module-scoped fixture is used) instead of "created fresh
for each test run", and reflow/wrap the long sentence that describes certificate
creation so no line exceeds 99 characters; locate the docstring referencing
get_cert_info(cert) and the module-scoped cert fixture and adjust the wording
and line breaks accordingly.
---
Nitpick comments:
In `@tests/core/lib/test_ssl.py`:
- Around line 639-649: Replace the brittle length-and-character checks in
test_activation_date_format and test_expiration_date_format with strict parsing:
call get_cert_info(self_signed_cert_pem) as before, then use
datetime.strptime(result["activation_date"], "%Y-%m-%d") and
datetime.strptime(result["expiration_date"], "%Y-%m-%d") respectively to assert
they parse successfully (import datetime at top if missing); keep references to
the test functions and get_cert_info to locate and update the assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 01946007-f26c-4d34-b862-83e50ebff44f
📒 Files selected for processing (1)
tests/core/lib/test_ssl.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 631-633: The test fails because get_cert_info builds subject from
all RDNs (x509.get_subject().get_components()) so result["subject"] contains
"C=US, CN=test.example.com" not just "test.example.com"; fix the test in
test_subject_contains_cn by asserting the CN is present (e.g., assert
"CN=test.example.com" in result["subject"]) or update the expectation to the
full formatted subject string, or alternatively change get_cert_info to extract
and return only the CN from the subject (use get_components() to find the
component with name b"CN" and decode its value).
🪄 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: 6f151444-5583-4e78-9c52-e43122a82a79
📒 Files selected for processing (1)
tests/core/lib/test_ssl.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
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 13 unit tests for
get_cert_info()innettacker/core/lib/ssl.pyas a newTestGetCertInfoclass appended to the existing test suite. Original 574-line SSL test site is fully preserved##What was tested
Approach
Tests use a real self-signed certificate generated with pyOpenSSL (sha256, valid 2026-04-01 to 2027-04-01). Two tests mock
is_weak_hash_algoviapatch("nettacker.core.lib.ssl.is_weak_hash")to isolate the weak signing logo field from the actual hash logic, as suggested in the PR#1453 review.Type of change
Checklist
make pre-commitand confirm it didn't generate any warnings/changesmake test, I confirm all tests passed locallydocs/folder