-
-
Notifications
You must be signed in to change notification settings - Fork 952
Improve SSL handshake error visibility in ssl_version_and_cipher_scan #1185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughPort-to-service resolution was added and SSL certificate retrievals are wrapped in try/except; failures log info, set Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.40.0)nettacker/web/static/report/d3_tree_v1.htmlThanks 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nettacker/core/lib/ssl.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use 4-space indents in Python code
Limit lines to 99 characters (ruff/ruff-format/isort profile=black)
Module and file names should use lower_snake_case
Function and variable names should use lower_snake_case
Class names should use PascalCase
Constants should use UPPER_SNAKE_CASE
Keep functions small and add type hints where practical
Files:
nettacker/core/lib/ssl.py
nettacker/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Add docstrings for public APIs in the nettacker package
Files:
nettacker/core/lib/ssl.py
nettacker/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Place core libraries under nettacker/core/
Files:
nettacker/core/lib/ssl.py
nettacker/core/lib/ssl.py
Outdated
| try: | ||
| cert = ssl.get_server_certificate((host, port)) | ||
| except ssl.SSLError: | ||
| cert = None | ||
| except socket.gaierror: | ||
| except ssl.SSLError as e: | ||
| log.debug( | ||
| "SSL handshake failed while fetching certificate for %s:%s: %s", | ||
| host, | ||
| port, | ||
| e, | ||
| ) | ||
| return { | ||
| "status": "error", | ||
| "reason": "ssl_handshake_failed", | ||
| "ssl_flag": True, | ||
| "peer_name": peer_name, | ||
| "service": socket.getservbyport(int(port)), | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical: Error response breaks condition evaluation logic.
The error response returns ssl_flag=True but omits keys like weak_version, cipher_suite, weak_cipher_suite, etc. The response_conditions_matched method (line 245) checks if response and response["ssl_flag"] then directly accesses response[condition] (lines 252-262, 273-276). When conditions reference missing keys, this will raise KeyError.
Choose one of these solutions:
Option 1 (recommended): Guard the condition evaluation
def response_conditions_matched(self, sub_step, response):
conditions = sub_step["response"]["conditions"]
condition_type = sub_step["response"]["condition_type"]
condition_results = {}
if sub_step["method"] in {
"ssl_certificate_scan",
"ssl_version_and_cipher_scan",
}:
- if response and response["ssl_flag"]:
+ if response and response["ssl_flag"] and response.get("status") != "error":
for condition in conditions:Option 2: Set ssl_flag=False in the error response
return {
"status": "error",
"reason": "ssl_handshake_failed",
- "ssl_flag": True,
+ "ssl_flag": False,
"peer_name": peer_name,
"service": socket.getservbyport(int(port)),
}Option 3: Include all expected keys in the error response
return {
"status": "error",
"reason": "ssl_handshake_failed",
+ "ssl_version": [],
+ "weak_version": False,
+ "cipher_suite": [],
+ "weak_cipher_suite": False,
+ "issuer": "NA",
+ "subject": "NA",
+ "expiration_date": "NA",
"ssl_flag": True,
"peer_name": peer_name,
"service": socket.getservbyport(int(port)),
}Minor: socket.getservbyport may raise OSError.
Line 200 calls socket.getservbyport(int(port)) which can raise OSError if the port has no known service name. This would escape the exception handler. Consider wrapping it or using a default value.
- "service": socket.getservbyport(int(port)),
+ "service": socket.getservbyport(int(port)) if port in range(1, 65536) else "unknown",Or catch the exception:
+ try:
+ service = socket.getservbyport(int(port))
+ except OSError:
+ service = "unknown"
return {
"status": "error",
"reason": "ssl_handshake_failed",
"ssl_flag": True,
"peer_name": peer_name,
- "service": socket.getservbyport(int(port)),
+ "service": service,
}
nettacker/core/lib/ssl.py
Outdated
| cert = None | ||
| except socket.gaierror: | ||
| except ssl.SSLError as e: | ||
| log.debug( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't have a .debug() in our logger (its a custom logger btw). Please run the code before submitting a PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You’re right. I missed that Nettacker uses a custom logger, not the standard logging interface.
I’ve identified two issues here and will address both in the next update:
- Replace the debug calls with the appropriate logger method used across the codebase.
- Remove the early error return and instead keep the execution flow consistent by setting cert=None and adjusting ssl_flag, so condition evaluation and response shape remain backward-compatible.
I’ll push an update shortly after validating the flow locally.
nettacker/core/lib/ssl.py
Outdated
| "service": socket.getservbyport(int(port)), | ||
| } | ||
| except socket.gaierror as e: | ||
| log.debug( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
nettacker/core/lib/ssl.py (4)
176-187: Exception handling improves error visibility, but consider catching additional exceptions.The try/except wrapper properly handles
ssl.SSLErrorandsocket.gaierror, logs failures at the appropriate level, and setsssl_flag=Falseto prevent downstreamKeyErrorin condition evaluation (line 254). This aligns well with the PR's goal of improving error visibility.However,
ssl.get_server_certificatecan also raisesocket.timeoutandOSErrorfor network issues. Consider catching these as well for more robust error handling.Apply this diff to handle additional exceptions:
- except (ssl.SSLError, socket.gaierror) as e: + except (ssl.SSLError, socket.gaierror, socket.timeout, OSError) as e: log.info( "Failed to fetch SSL certificate for %s:%s: %s", host, port, e, ) scan_info["ssl_flag"] = False
208-217: Exception handling is appropriate; consider catching additional network exceptions.The error handling logs certificate fetch failures and sets
cert=Noneandssl_flag=False, allowing the scan to continue and gather SSL version/cipher information even when certificate retrieval fails. The conditional handling at line 219 (cert_info = get_cert_info(cert) if cert else None) and lines 228-230 properly manages theNonecert case.As with
ssl_certificate_scan, consider catchingsocket.timeoutandOSErrorfor comprehensive network error handling.Apply this diff:
- except (ssl.SSLError, socket.gaierror) as e: + except (ssl.SSLError, socket.gaierror, socket.timeout, OSError) as e: log.info( "Failed to fetch SSL certificate for %s:%s: %s", host, port, e, ) cert = None ssl_flag = False
156-156: Add docstring for public API method.Per coding guidelines, public APIs in the nettacker package should have docstrings. Consider documenting the purpose, parameters, and return value.
Example docstring:
def ssl_certificate_scan(self, host, port, timeout): """ Scan SSL certificate information for the given host and port. Args: host: Target hostname or IP address port: Target port number timeout: Connection timeout in seconds Returns: dict: Scan results containing ssl_flag, peer_name, service, and certificate info (if SSL is enabled and certificate fetch succeeds), or None if connection fails """
192-192: Add docstring for public API method.This public API method also needs a docstring per coding guidelines.
Example docstring:
def ssl_version_and_cipher_scan(self, host, port, timeout): """ Scan SSL version, cipher suites, and certificate information. Args: host: Target hostname or IP address port: Target port number timeout: Connection timeout in seconds Returns: dict: Scan results with ssl_flag, service, peer_name, and if SSL is enabled: ssl_version, weak_version, cipher_suite, weak_cipher_suite, issuer, subject, and expiration_date. Returns None if connection fails. """
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nettacker/core/lib/ssl.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use 4-space indents in Python code
Limit lines to 99 characters (ruff/ruff-format/isort profile=black)
Module and file names should use lower_snake_case
Function and variable names should use lower_snake_case
Class names should use PascalCase
Constants should use UPPER_SNAKE_CASE
Keep functions small and add type hints where practical
Files:
nettacker/core/lib/ssl.py
nettacker/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Add docstrings for public APIs in the nettacker package
Files:
nettacker/core/lib/ssl.py
nettacker/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Place core libraries under nettacker/core/
Files:
nettacker/core/lib/ssl.py
🔇 Additional comments (1)
nettacker/core/lib/ssl.py (1)
164-167: Safe service name resolution looks good.The try/except wrapper around
socket.getservbyportwith fallback to"unknown"properly handles theOSErrorthat can be raised for unrecognized ports. This pattern is consistently applied in bothssl_certificate_scanandssl_version_and_cipher_scan(lines 200-203).
nettacker/core/lib/ssl.py
Outdated
| cert_info = get_cert_info(cert) | ||
| scan_info = cert_info | scan_info | ||
| except (ssl.SSLError, socket.gaierror) as e: | ||
| log.info( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also is not going to work as .info() expects only 1 parameter as an argument. Check nettacker/logger.py. If you want to format, please use f-strings.
nettacker/core/lib/ssl.py
Outdated
| cert = None | ||
| except socket.gaierror: | ||
| except (ssl.SSLError, socket.gaierror) as e: | ||
| log.info( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
nettacker/core/lib/ssl.py (2)
156-187: Add docstring for this public API method.Per coding guidelines, public APIs in the nettacker package require docstrings. Document the parameters (host, port, timeout), return value structure, and key behaviors (especially the ssl_flag semantics when certificate fetch fails).
📋 Example docstring structure
def ssl_certificate_scan(self, host, port, timeout): """ Scan SSL certificate information for a given host and port. Args: host: Target hostname or IP address port: Target port number timeout: Connection timeout in seconds Returns: dict: Scan results containing ssl_flag, peer_name, service, and certificate details (issuer, subject, expiration, etc.) when SSL is present. Returns None if connection fails. Note: ssl_flag is set to False if SSL handshake or certificate fetch fails. """
189-234: Add docstring for this public API method.Per coding guidelines, public APIs in the nettacker package require docstrings. Document the parameters, return value structure (especially the different response shapes based on ssl_flag), and the behavior when certificate fetch fails.
📋 Example docstring structure
def ssl_version_and_cipher_scan(self, host, port, timeout): """ Scan SSL/TLS version and cipher suite information for a given host and port. Args: host: Target hostname or IP address port: Target port number timeout: Connection timeout in seconds Returns: dict: Scan results including ssl_version, cipher_suite, weak_version, weak_cipher_suite, and certificate details when SSL is present. Returns minimal dict with ssl_flag=False when SSL is not detected. Returns None if connection fails. Note: ssl_flag is set to False if certificate fetch fails, but SSL version and cipher suite data are still collected. """
🧹 Nitpick comments (2)
nettacker/core/lib/ssl.py (2)
176-184: Good error handling, but ssl_flag semantics warrant a comment.The error handling correctly catches both ssl.SSLError and socket.gaierror, logs appropriately using f-strings, and sets ssl_flag to False. However, setting ssl_flag to False when SSL is present but the certificate cannot be fetched is semantically unusual. Consider adding an inline comment explaining this design choice—it prevents condition evaluation (line 248) from accessing potentially missing certificate fields.
203-210: Error handling is correct and consistent.The error handling correctly catches both ssl.SSLError and socket.gaierror, uses f-strings for logging, and sets ssl_flag to False. The approach is consistent with ssl_certificate_scan.
One observation: after this error, the code still calls is_weak_ssl_version and is_weak_cipher_suite (lines 214-215), which make additional SSL connection attempts. Since condition evaluation is skipped when ssl_flag is False (line 248), this data is primarily for diagnostics. If these calls frequently fail after cert fetch failures, you might consider guarding them with an additional condition, though the current approach provides more complete diagnostic data.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nettacker/core/lib/ssl.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use 4-space indents in Python code
Limit lines to 99 characters (ruff/ruff-format/isort profile=black)
Module and file names should use lower_snake_case
Function and variable names should use lower_snake_case
Class names should use PascalCase
Constants should use UPPER_SNAKE_CASE
Keep functions small and add type hints where practical
Files:
nettacker/core/lib/ssl.py
nettacker/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Add docstrings for public APIs in the nettacker package
Files:
nettacker/core/lib/ssl.py
nettacker/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Place core libraries under nettacker/core/
Files:
nettacker/core/lib/ssl.py
🔇 Additional comments (3)
nettacker/core/lib/ssl.py (3)
164-167: LGTM - proper error handling for service lookup.The try/except wrapper correctly handles the OSError that socket.getservbyport can raise for unknown ports.
213-213: LGTM - safe conditional cert_info handling.Correctly guards against calling get_cert_info when cert is None.
217-234: LGTM - complete response structure prevents KeyError.The response structure includes all expected fields with appropriate "NA" fallbacks for certificate-related fields when cert_info is None. This design prevents the KeyError issue during condition evaluation (line 248-287) that was flagged in previous reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ADOPTERS.md (1)
32-32: Reduce exclamation mark frequency for a more professional tone.The line contains 5 exclamation marks across 1,298 characters, which may come across as overly enthusiastic for formal documentation. Consider reducing to 1–2 for better balance.
🔎 Suggested revision
-Thanks to everyone using and contributing to OWASP Nettacker! We appreciate your support and feedback. +Thanks to everyone using and contributing to OWASP Nettacker. We appreciate your support and feedback.Or, if emphasis is desired:
-Thanks to everyone using and contributing to OWASP Nettacker! We appreciate your support and feedback. +Thanks to everyone using and contributing to OWASP Nettacker! We appreciate your support and feedback
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
nettacker/web/static/js/bootstrap-select.min.jsis excluded by!**/*.min.jsnettacker/web/static/js/bootstrap-tagsinput-angular.min.jsis excluded by!**/*.min.jsnettacker/web/static/js/bootstrap-tagsinput.min.jsis excluded by!**/*.min.jsnettacker/web/static/js/bootstrap.min.jsis excluded by!**/*.min.jsnettacker/web/static/js/d3.v4.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (57)
ADOPTERS.mddocs/API.mddocs/Contributors.mddocs/Media.mddocs/Usage.mddocs/index.mdnettacker/api/readme.mdnettacker/core/lib/ssl.pynettacker/core/readme.mdnettacker/lib/graph/d3_tree_v1/readme.mdnettacker/lib/graph/d3_tree_v2/readme.mdnettacker/lib/graph/readme.mdnettacker/lib/html_log/readme.mdnettacker/lib/icmp/readme.mdnettacker/lib/payloads/User-Agents/web_browsers_user_agents.txtnettacker/lib/payloads/readme.mdnettacker/lib/payloads/wordlists/admin_wordlist.txtnettacker/lib/payloads/wordlists/config_wordlist.txtnettacker/lib/payloads/wordlists/dir_wordlist.txtnettacker/lib/payloads/wordlists/pma_wordlist.txtnettacker/lib/payloads/wordlists/wp_plugin_small.txtnettacker/lib/payloads/wordlists/wp_theme_small.txtnettacker/lib/payloads/wordlists/wp_timethumbs.txtnettacker/locale/hi.yamlnettacker/locale/it.yamlnettacker/locale/ja.yamlnettacker/locale/readme.mdnettacker/modules/brute/pop3.yamlnettacker/modules/brute/pop3s.yamlnettacker/modules/scan/confluence_version.yamlnettacker/modules/scan/icmp.yamlnettacker/modules/vuln/adobe_coldfusion_cve_2023_26360.yamlnettacker/modules/vuln/citrix_cve_2019_19781.yamlnettacker/modules/vuln/citrix_cve_2023_4966.yamlnettacker/modules/vuln/confluence_cve_2023_22515.yamlnettacker/modules/vuln/confluence_cve_2023_22527.yamlnettacker/modules/vuln/exponent_cms_cve_2021_38751.yamlnettacker/modules/vuln/f5_cve_2020_5902.yamlnettacker/modules/vuln/msexchange_cve_2021_26855.yamlnettacker/modules/vuln/msexchange_cve_2021_34473.yamlnettacker/modules/vuln/sonicwall_sslvpn_cve_2024_53704.yamlnettacker/modules/vuln/ssl_certificate_weak_signature.yamlnettacker/modules/vuln/ssl_self_signed_certificate.yamlnettacker/modules/vuln/ssl_weak_cipher.yamlnettacker/modules/vuln/subdomain_takeover.yamlnettacker/modules/vuln/wp_plugin_cve_2023_47668.yamlnettacker/web/readme.mdnettacker/web/static/css/animate.min.cssnettacker/web/static/css/bootstrap-select.min.cssnettacker/web/static/css/bootstrap.min.cssnettacker/web/static/index.htmlnettacker/web/static/js/buttons.jsnettacker/web/static/report/compare_report.htmlnettacker/web/static/report/d3_tree_v1.htmlnettacker/web/static/report/html_table.cssnettacker/web/static/report/json_parse.jsnettacker/web/static/report/table_items.html
✅ Files skipped from review due to trivial changes (43)
- nettacker/modules/vuln/ssl_certificate_weak_signature.yaml
- nettacker/modules/vuln/confluence_cve_2023_22515.yaml
- nettacker/locale/ja.yaml
- nettacker/modules/brute/pop3.yaml
- nettacker/lib/payloads/wordlists/admin_wordlist.txt
- nettacker/web/static/index.html
- nettacker/web/static/js/buttons.js
- nettacker/modules/vuln/f5_cve_2020_5902.yaml
- nettacker/lib/payloads/wordlists/pma_wordlist.txt
- nettacker/modules/vuln/subdomain_takeover.yaml
- nettacker/lib/icmp/readme.md
- docs/API.md
- nettacker/modules/vuln/citrix_cve_2019_19781.yaml
- nettacker/modules/vuln/adobe_coldfusion_cve_2023_26360.yaml
- nettacker/modules/vuln/citrix_cve_2023_4966.yaml
- nettacker/lib/graph/d3_tree_v1/readme.md
- nettacker/modules/vuln/msexchange_cve_2021_34473.yaml
- nettacker/locale/it.yaml
- nettacker/web/readme.md
- nettacker/api/readme.md
- nettacker/modules/vuln/msexchange_cve_2021_26855.yaml
- docs/Media.md
- nettacker/lib/payloads/wordlists/dir_wordlist.txt
- docs/index.md
- nettacker/modules/scan/icmp.yaml
- nettacker/web/static/report/d3_tree_v1.html
- nettacker/modules/vuln/sonicwall_sslvpn_cve_2024_53704.yaml
- nettacker/locale/hi.yaml
- nettacker/modules/vuln/confluence_cve_2023_22527.yaml
- nettacker/modules/vuln/exponent_cms_cve_2021_38751.yaml
- nettacker/modules/vuln/ssl_weak_cipher.yaml
- nettacker/lib/payloads/wordlists/wp_timethumbs.txt
- nettacker/modules/vuln/wp_plugin_cve_2023_47668.yaml
- docs/Usage.md
- nettacker/modules/brute/pop3s.yaml
- nettacker/modules/vuln/ssl_self_signed_certificate.yaml
- nettacker/lib/payloads/wordlists/wp_theme_small.txt
- nettacker/lib/payloads/User-Agents/web_browsers_user_agents.txt
- nettacker/locale/readme.md
- nettacker/web/static/report/table_items.html
- nettacker/modules/scan/confluence_version.yaml
- nettacker/lib/payloads/wordlists/config_wordlist.txt
- nettacker/lib/html_log/readme.md
🚧 Files skipped from review as they are similar to previous changes (1)
- nettacker/core/lib/ssl.py
🧰 Additional context used
📓 Path-based instructions (3)
nettacker/web/static/**
📄 CodeRabbit inference engine (AGENTS.md)
Store web UI static assets under nettacker/web/static/
Files:
nettacker/web/static/report/compare_report.htmlnettacker/web/static/css/bootstrap-select.min.cssnettacker/web/static/report/html_table.cssnettacker/web/static/report/json_parse.js
nettacker/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Place core libraries under nettacker/core/
Files:
nettacker/core/readme.md
docs/**
📄 CodeRabbit inference engine (AGENTS.md)
Place documentation under docs/
Files:
docs/Contributors.md
🧠 Learnings (1)
📚 Learning: 2025-09-07T19:20:58.332Z
Learnt from: CR
Repo: OWASP/Nettacker PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-07T19:20:58.332Z
Learning: Applies to nettacker.py : Repository provides nettacker.py as an entry script (use as Python entry point)
Applied to files:
ADOPTERS.md
🪛 Biome (2.1.2)
nettacker/web/static/css/bootstrap-select.min.css
[error] 6-6: Unexpected unknown unit: px\9
See MDN web docs for more details.
Use a known unit instead, such as:
(lint/correctness/noUnknownUnit)
[error] 6-6: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
outline is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
nettacker/web/static/report/html_table.css
[error] 98-99: Expected a qualified rule, or an at rule but instead found '</'.
Expected a qualified rule, or an at rule here.
(parse)
[error] 99-99: Expected a compound selector but instead found the end of the file.
Expected a compound selector here.
(parse)
nettacker/web/static/report/json_parse.js
[error] 14-14: Unexpected token. Did you mean {'}'} or }?
(parse)
🪛 LanguageTool
ADOPTERS.md
[style] ~32-~32: Using many exclamation marks might seem excessive (in this case: 5 exclamation marks for a text that’s 1298 characters long)
Context: ...sing and contributing to OWASP Nettacker! We appreciate your support and feedback...
(EN_EXCESSIVE_EXCLAMATION)
docs/Contributors.md
[style] ~38-~38: Consider using a different adverb to strengthen your wording.
Context: ... The OWASP Nettacker Project Team is very grateful to Google's Summer of Code (GS...
(VERY_DEEPLY)
🔇 Additional comments (7)
docs/Contributors.md (1)
38-38: No action required.This file contains only a trailing newline adjustment—a formatting normalization that has no semantic or functional impact. The documentation content is unchanged.
nettacker/core/readme.md (1)
20-20: No review needed—formatting artifact only.Per the AI summary, the utility.py entry is removed and re-added with identical content, resulting in no functional or semantic changes to the readme. This appears to be a whitespace/formatting-only adjustment (e.g., EOL normalization).
nettacker/lib/payloads/wordlists/wp_plugin_small.txt (1)
291-291:⚠️ Critical inconsistency: File under review does not match PR objectives.The PR objectives describe modifications to
nettacker/core/lib/ssl.pyfor improving SSL handshake error visibility and certificate retrieval failure logging. However, the file provided (nettacker/lib/payloads/wordlists/wp_plugin_small.txt) is a static WordPress plugin wordlist with only a trailing newline addition and one new entry ("gutentor").Please clarify:
- Is this the correct file for this PR?
- Are the SSL handshake improvements in a separate file not yet provided?
- Does this PR bundle unrelated changes (SSL improvements + wordlist updates)?
nettacker/web/static/css/bootstrap-select.min.css (1)
1-6: Incidental whitespace change to minified 3rd-party CSS asset.This file contains only a trailing newline adjustment to a minified Bootstrap Select CSS library (v1.12.4). The change is unrelated to the PR's core objective (SSL handshake error visibility in ssl.py) and appears incidental.
File location complies with coding guidelines for web UI static assets. The static analysis warnings (unknown
px\9unit, duplicateoutlineproperties) are artifacts of minified code and should not be addressed in the output file; if improvements are needed, they should be made to the source CSS before re-minification.nettacker/web/static/report/html_table.css (1)
98-99: Formatting change: trailing newline added.This is a standard code style improvement (POSIX convention). The Biome parse errors flagged by static analysis are false positives—the tool is confused by the HTML closing tag
</style>embedded in the file.Note: This change doesn't relate to the PR's stated objectives (SSL handshake error visibility improvements).
nettacker/web/static/report/json_parse.js (1)
14-14: Formatting change: trailing newline added.This is a standard code style improvement. The Biome parse error is a false positive—it's confused by the HTML closing tag
</script>in the file.Note: This change doesn't relate to the PR's stated objectives (SSL handshake error visibility improvements).
nettacker/web/static/report/compare_report.html (1)
220-220: Formatting change: trailing newline added.This is a standard code style improvement consistent with the other web asset formatting changes in this PR. No functional impact.
Note: These web asset formatting changes don't relate to the PR's stated objectives (SSL handshake error visibility improvements in
nettacker/core/lib/ssl.py).
Summary
This PR improves the visibility of SSL handshake–related failures in
ssl_version_and_cipher_scanby avoiding silent exception handling and providing minimal, structured context when certificate retrieval fails.What changed
Why this change
Previously, SSL handshake errors during certificate retrieval were silently swallowed, which made it difficult to distinguish between:
This small change improves debuggability without altering scan logic or engine behavior.
Scope and compatibility
nettacker/core/lib/ssl.pyRelated