Add TestIsWeakHashAlgo tests for is_weak_hash_algo() in ssl.py#1452
Add TestIsWeakHashAlgo tests for is_weak_hash_algo() in ssl.py#1452Raavi29 wants to merge 10 commits intoOWASP:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughAdded a new test class 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/core/lib/test_ssl.py (1)
575-623: Good test coverage foris_weak_hash_algo().The new tests add valuable coverage for RSA suffix variants, case insensitivity, additional safe algorithms (sha384, sha512), and edge cases. The docstring and section comments are helpful.
Note that there's an existing parameterized test at lines 457-469 in
TestSslMethodthat covers basic cases. Consider consolidating by extending that parameterized test with your new cases, which would reduce duplication and keep related tests together:`@pytest.mark.parametrize`( "algo,expected", [ # Weak algorithms ("md2", True), ("md4", True), ("md5", True), ("sha1", True), ("sha1WithRSAEncryption", True), ("md5WithRSAEncryption", True), ("SHA1WithRSAEncryption", True), # case insensitivity ("MD5WithRSAEncryption", True), # Safe algorithms ("sha256", False), ("sha256WithRSAEncryption", False), ("sha384WithRSAEncryption", False), ("sha512WithRSAEncryption", False), # Edge cases ("", False), ("someRandomAlgorithm", False), ], ) def test_is_weak_hash_algo(self, algo, expected): assert is_weak_hash_algo(algo) == expectedHowever, keeping a separate class for organizational clarity is also acceptable.
🤖 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 575 - 623, The new TestIsWeakHashAlgo tests duplicate coverage of is_weak_hash_algo already parameterized in TestSslMethod; update the existing pytest.mark.parametrize in TestSslMethod to include the RSA-suffix, case-insensitive, safe-algorithm and edge-case tuples (e.g., ("sha1WithRSAEncryption", True), ("md5WithRSAEncryption", True), ("SHA1WithRSAEncryption", True), ("MD5WithRSAEncryption", True), ("sha384WithRSAEncryption", False), ("sha512WithRSAEncryption", False), ("", False), ("someRandomAlgorithm", False)) and then remove or collapse the separate TestIsWeakHashAlgo class to avoid duplication while keeping the same assertions (assert is_weak_hash_algo(algo) == expected).
🤖 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 622-623: The file ends without a trailing newline; open
tests/core/lib/test_ssl.py and add a single newline character at the end of the
file (after the closing line containing the test_random_string_is_not_weak
assertion) so the file terminates with a newline; no code changes to functions
(is_weak_hash_algo or test_random_string_is_not_weak) are required—just ensure
the EOF has a newline.
---
Nitpick comments:
In `@tests/core/lib/test_ssl.py`:
- Around line 575-623: The new TestIsWeakHashAlgo tests duplicate coverage of
is_weak_hash_algo already parameterized in TestSslMethod; update the existing
pytest.mark.parametrize in TestSslMethod to include the RSA-suffix,
case-insensitive, safe-algorithm and edge-case tuples (e.g.,
("sha1WithRSAEncryption", True), ("md5WithRSAEncryption", True),
("SHA1WithRSAEncryption", True), ("MD5WithRSAEncryption", True),
("sha384WithRSAEncryption", False), ("sha512WithRSAEncryption", False), ("",
False), ("someRandomAlgorithm", False)) and then remove or collapse the separate
TestIsWeakHashAlgo class to avoid duplication while keeping the same assertions
(assert is_weak_hash_algo(algo) == expected).
🪄 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: bf035ee3-8d3c-4105-9fdb-c077c737c629
📒 Files selected for processing (1)
tests/core/lib/test_ssl.py
- 14 tests covering: expired cert, valid cert, self-signed detection, date format validation, key presence, expiring_soon logic - Uses cryptography library to generate fake certs — no network required - ssl.py coverage: 15% to 17% - Addresses CodeRabbit feedback from PR OWASP#1452
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.
|
Latest commit adds a test for weak algo not at start of string (rsaWithSHA1Encryption) - addresses CodeRabbit suggestion about non-prefix cases. |
There was a problem hiding this comment.
Pull request overview
Adds additional unit tests for is_weak_hash_algo() (in nettacker/core/lib/ssl.py) to improve coverage around algorithm string variants, casing, and edge inputs.
Changes:
- Appends a new
TestIsWeakHashAlgotest class covering weak/safe algorithm variants and case-insensitivity. - Adds edge-case tests for empty and unrecognized algorithm strings.
- Minor reordering within the
from nettacker.core.lib.ssl import (...)list.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| class TestIsWeakHashAlgo: | ||
| """ | ||
| Tests for is_weak_hash_algo(algo). | ||
| This function returns True if the algorithm is considered weak | ||
| (md2, md4, md5, sha1), and False if it is safe (sha256, sha512 etc.) | ||
| """ |
There was a problem hiding this comment.
PR description says this adds 11 tests, but TestIsWeakHashAlgo defines 12 new test methods (4 weak + 2 uppercase + 3 safe + 3 edge). Also, this file already has an is_weak_hash_algo test earlier, so the stated coverage delta / “no dedicated tests” may be outdated. Please update the PR description to match the actual changes.
| class TestIsWeakHashAlgo: | ||
| """ | ||
| Tests for is_weak_hash_algo(algo). | ||
| This function returns True if the algorithm is considered weak | ||
| (md2, md4, md5, sha1), and False if it is safe (sha256, sha512 etc.) | ||
| """ | ||
|
|
||
| # --- WEAK algorithms — should return 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 | ||
|
|
There was a problem hiding this comment.
This file already contains a parametrized test_is_weak_hash_algo earlier. The new TestIsWeakHashAlgo class overlaps with several of those cases (md2/md4/md5/sha1) and adds variants. Consider folding these new inputs into the existing parametrized test to reduce duplication and keep related assertions in one place.
|
@Raavi29 PRs unsigned commits will not be accepted |
Proposed change
Summary
Adds 11 unit tests for
is_weak_hash_algo()innettacker/core/lib/ssl.pyas a newTestIsWeakHashAlgoclass appended to the existing test suite.What was tested
Notes
is_weak_hash_algo()coverage: 0% -> 15% on nettacker/core/lib/ssl.pyType of change
Checklist
make pre-commitand confirm it didn't generate any warnings/changesmake test, I confirm all tests passed locallydocs/folder