Add STARTTLS support to testssl-inspector - #197
Conversation
Code Review by Qodo
1.
|
📝 WalkthroughWalkthroughSTARTTLS support is added to the ChangesSTARTTLS Support for testssl-inspector
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
PR Summary by QodoAdd STARTTLS support to testssl-inspector scan wrapper WalkthroughsDescription• Add --starttls= to enable scanning STARTTLS services via testssl.sh. • Validate allowed STARTTLS protocols and default ports; preserve explicit target ports. • Document STARTTLS usage and examples in README and scan command docs. Diagramgraph TD
A["scan.js (CLI)"] --> B["parseArgs()"] --> C["STARTTLS ports map"] --> D["withDefaultStarttlsPort()"] --> E["resolveRunner()/testsslArgs()"] --> F["testssl.sh"]
High-Level AssessmentThe following are alternative approaches to this PR: 1. Pass through `--starttls` without protocol allowlist/port defaults
2. Use a host/port parser that handles IPv6 literals explicitly
Recommendation: Current approach (explicit allowlist + default-port inference + passthrough to testssl) is a good UX/robustness balance. Consider tightening target parsing for IPv6 (':' detection) if IPv6 targets are in scope, and remove/avoid unused parameters (e.g., File ChangesEnhancement (1)
Documentation (2)
|
Greptile SummaryThis PR wires
Confidence Score: 4/5Safe to merge the STARTTLS logic itself; the unresolved docker argv/__jsonPath gap from the previous review still means docker-runner scans fail at JSON read time. The new STARTTLS parsing, port defaulting, URI generation, and normalizeTargetFindings threading are all correct and well-tested. The docker runner argv closure spreads testsslArgs() into a new array literal, silently discarding the __jsonPath non-index property — docker scans throw 'testssl produced no parseable JSON at undefined' on every run. Files Needing Attention: plugins/connectors/testssl-inspector/scripts/scan.js — specifically the docker argv closure in resolveRunner where ...testsslArgs(...) spread drops __jsonPath
|
| Filename | Overview |
|---|---|
| plugins/connectors/testssl-inspector/scripts/scan.js | Core STARTTLS implementation: adds protocol allowlist, default port injection, --starttls flag threading, IPv6-aware parseTarget/hasExplicitPort, and protocol-specific URI scheme in endpointUri. The testsslArgs hostOutDir path-split fix is correct in isolation, but the docker argv closure still spreads the result, silently dropping __jsonPath. |
| tests/testssl-inspector-starttls.test.mjs | Adds six targeted tests covering protocol allowlist, port defaulting (including IPv6 brackets), CLI arg generation, docker path split, and URI scheme for both STARTTLS and IPv6 targets. |
| plugins/connectors/testssl-inspector/README.md | Updated scope section lists all seven STARTTLS protocols with default ports and clarifies implicit-TLS services. |
| plugins/connectors/testssl-inspector/commands/scan.md | Adds --starttls option documentation, updated output format doc, and four new usage examples. |
| plugins/connectors/testssl-inspector/skills/testssl-inspector-expert/SKILL.md | Updated resource.uri and metadata field descriptions to reflect the new starttls+proto:// scheme and effective_target/starttls metadata fields. |
Reviews (5): Last reviewed commit: "Merge branch 'main' into fix-165-starttl..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/connectors/testssl-inspector/scripts/scan.js`:
- Around line 230-234: The withDefaultStarttlsPort function incorrectly detects
explicit ports by checking if target.includes(':'), which treats IPv6 addresses
as having explicit ports even when they don't (e.g., [2001:db8::1] has colons
but no port). Fix the port detection logic to properly handle IPv6 literals in
square brackets: if the target starts with '[', check whether there's a colon
after the closing bracket ']' to determine if a port is specified; otherwise,
check for the presence of a colon to determine if a port exists. This ensures
the default port is correctly appended only when no explicit port is provided.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 84983c7b-e419-4c9f-94bc-9b7adf3c4081
📒 Files selected for processing (3)
plugins/connectors/testssl-inspector/README.mdplugins/connectors/testssl-inspector/commands/scan.mdplugins/connectors/testssl-inspector/scripts/scan.js
Co-authored-by: Ethan Troy <ethanolivertroy@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/connectors/testssl-inspector/scripts/scan.js`:
- Around line 88-90: `metadata.target` is being populated with the expanded
`finalTarget` instead of the user-provided input, causing it to diverge from the
original target value. Update the flow in `scan.js` so `runTestssl` still uses
`finalTarget` for execution, but `normalizeTargetFindings` receives the original
`target` separately and stores that in `metadata.target`. Adjust the
`normalizeTargetFindings` signature and call sites accordingly, using the
existing `finalTarget`, `target`, and `normalizeTargetFindings` symbols to keep
host/port handling and metadata consistent.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a6ff5fbe-a0fa-45f7-9163-06e40a84de65
📒 Files selected for processing (5)
plugins/connectors/testssl-inspector/README.mdplugins/connectors/testssl-inspector/commands/scan.mdplugins/connectors/testssl-inspector/scripts/scan.jsplugins/connectors/testssl-inspector/skills/testssl-inspector-expert/SKILL.mdtests/testssl-inspector-starttls.test.mjs
✅ Files skipped from review due to trivial changes (1)
- plugins/connectors/testssl-inspector/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/connectors/testssl-inspector/commands/scan.md
Co-authored-by: Ethan Troy <ethanolivertroy@users.noreply.github.com>
…TLS errors - Remove the unused `starttls` parameter from runTestssl(); the flag is already baked into the runner's argv closure, so the argument had no effect (addresses Greptile review on PR GRCEngClub#197). - Reject implicit-TLS service names (smtps, imaps, pop3s, ftps, ldaps) with a precise hint to scan them as plain TLS on their implicit-TLS port, rather than the generic "unknown STARTTLS protocol" message. - Document metadata.effective_target in the expert SKILL.md so the docs match the emitted finding shape. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Hi @ethanolivertroy, I reviewed the automated feedback and pushed a focused follow-up in commit e201220. The update removes a dead parameter, clarifies the implicit-TLS handling for SMTPS on port 465, documents All checks pass: syntax validation, 6 targeted STARTTLS tests, and all 49 fixture-contract checks. I would appreciate a maintainer review when you have a chance. Thank you! |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Hi @ethanolivertroy, just following up on PR #197 since the final update has been ready since June 28. The PR remains mergeable, the targeted tests and all 49 fixture checks pass, and the upstream workflows appear to be awaiting maintainer authorization. When you have a chance, could you approve the workflows and review the PR? Thank you again. |
2 participants
Summary
Adds
--starttls=<proto>support to the testssl-inspector scan wrapper so users can scan STARTTLS services such as SMTP, IMAP, POP3, FTP, LDAP, Postgres, and MySQL.SMTPS on port 465 is implicit TLS, not STARTTLS — it is scanned as a normal TLS endpoint (
--target=host:465) without--starttls, so it is intentionally not part of the--starttlsallowlist. Passing--starttls=smtpsreturns a clear hint pointing at the implicit-TLS form.Changes
--starttls=<proto>inscan.jsEXIT.USAGEmail.example.com:587--starttls <proto>through totestssl.shcommands/scan.mdandREADME.mdVerification
Syntax check:
Result:
Contract fixture validation:
Result:
Happy-path STARTTLS argument parsing:
Result:
This confirms
--starttls=smtpis no longer rejected as an unknown flag. The command reaches runner/tool resolution, buttestssl.shis not installed locally.Explicit port preservation smoke test:
Result:
This confirms an explicit-port STARTTLS command is accepted and reaches runner/tool resolution.
Unknown protocol path:
Result:
Closes #165
Summary by CodeRabbit
starttls+<proto>://...) and include STARTTLS-related metadata.--starttls=<proto>options, and that implicit TLS services (e.g., SMTPS on 465) must be scanned as normal TLS endpoints.