Add STARTTLS support to testssl-inspector#197
Conversation
Code Review by Qodo
Context used✅ Compliance rules (platform):
17 rules 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 for non-docker usage; the --docker flag remains broken for STARTTLS scans (and was broken pre-PR) due to __jsonPath being lost through array spread in the docker argv closure. The non-docker path is correct end-to-end: protocol validation, port defaulting, testsslArgs construction, normalizeTargetFindings threading, and URI generation all work. The docker runner's argv closure spreads testsslArgs output into a new array, silently dropping the __jsonPath property, so runTestssl receives undefined as the JSON file path and throws on every docker scan. The new docker test passes because it calls testsslArgs() directly rather than exercising the argv closure. The resolveRunner docker branch in plugins/connectors/testssl-inspector/scripts/scan.js (lines ~196–208) needs __jsonPath re-attached to the constructed argv array after the spread. The corresponding test in tests/testssl-inspector-starttls.test.mjs should exercise runner.argv(target, mode).__jsonPath rather than testsslArgs().__jsonPath to catch this gap. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant CLI as CLI (scan.js main)
participant PA as parseArgs
participant RR as resolveRunner
participant TA as testsslArgs
participant RT as runTestssl
participant TS as testssl.sh / docker
participant NF as normalizeTargetFindings
CLI->>PA: "argv (--target, --starttls=smtp)"
PA-->>CLI: "args {targets, starttls:"smtp"}"
CLI->>RR: "{useDocker, configBinary, starttls:"smtp"}"
RR->>TA: (target, mode, "smtp") — baked into argv closure
TA-->>RR: base[] with __jsonPath set
RR-->>CLI: "runner {argv closure, cmd}"
loop for each target
CLI->>CLI: withDefaultStarttlsPort(target, "smtp") → finalTarget:25
CLI->>RT: runner, finalTarget, mode
RT->>RT: "argv = runner.argv(target, mode)"
Note over RT: argv.__jsonPath captured here
RT->>TS: spawn(cmd, argv) — includes --starttls smtp
TS-->>RT: JSON written to jsonPath
RT-->>CLI: raw JSON
CLI->>NF: "raw, finalTarget, starttls="smtp", originalTarget"
NF-->>CLI: Finding doc (uri: starttls+smtp://host:25/)
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant CLI as CLI (scan.js main)
participant PA as parseArgs
participant RR as resolveRunner
participant TA as testsslArgs
participant RT as runTestssl
participant TS as testssl.sh / docker
participant NF as normalizeTargetFindings
CLI->>PA: "argv (--target, --starttls=smtp)"
PA-->>CLI: "args {targets, starttls:"smtp"}"
CLI->>RR: "{useDocker, configBinary, starttls:"smtp"}"
RR->>TA: (target, mode, "smtp") — baked into argv closure
TA-->>RR: base[] with __jsonPath set
RR-->>CLI: "runner {argv closure, cmd}"
loop for each target
CLI->>CLI: withDefaultStarttlsPort(target, "smtp") → finalTarget:25
CLI->>RT: runner, finalTarget, mode
RT->>RT: "argv = runner.argv(target, mode)"
Note over RT: argv.__jsonPath captured here
RT->>TS: spawn(cmd, argv) — includes --starttls smtp
TS-->>RT: JSON written to jsonPath
RT-->>CLI: raw JSON
CLI->>NF: "raw, finalTarget, starttls="smtp", originalTarget"
NF-->>CLI: Finding doc (uri: starttls+smtp://host:25/)
end
|
![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.