TECH: SslScanner Improvement — Fix IMAP STARTTLS protocol in Test_SSL_Support

[EvoJocys]

Implements WI-1. Closes #8.

What

Fix the IMAP STARTTLS branch in Tool/Common/Test_SSL_Support.cs so SSL scans on port 143 actually work. The branch was a verbatim copy of the POP3 branch above it: it expected a POP3 +OK greeting and sent the POP3 STLS command. Per RFC 3501, IMAP servers greet with * OK; per RFC 2595, STARTTLS is a tagged IMAP command (<tag> STARTTLS). Every IMAP STARTTLS scan therefore threw InvalidOperationException("IMAP Server did not respond to connection request") unconditionally.

Why

The README advertises IMAP STARTTLS support but the feature has been broken since it was copy-pasted from the POP3 branch. Without this fix, anyone scanning an IMAP server on port 143 sees a meaningless error and concludes the tool is broken.

Score

Impact 4 × Confidence 5 ÷ Effort 1 = 20 (highest-Score row in the registry — broken advertised feature, ~10 line fix).

Changes

• Tool/Common/Test_SSL_Support.cs (port == 143 block):
  • Greeting check: +OK → * OK (RFC 3501 untagged OK).
  • STARTTLS command: STLS → a001 STARTTLS (RFC 2595 tagged).
  • Reply check: tag-tolerant IndexOf(" OK", OrdinalIgnoreCase) so a001 OK Begin TLS is accepted.
  • Both error messages updated to say STARTTLS instead of STLS.
• Tool/JocysCom.SslScanner.Tool.csproj: <Version>1.1.6</Version> → <Version>1.1.7</Version>.
• Tool/Documents/ChangeLog.txt: prepend dated v1.1.7 entry.
• .ai/work-item-improvement.md: flip WI-1 [Working] → [Done].

Verification

• C# compiles cleanly (standalone library build of the changed file passes; main project's PowerShell-only PreBuild step does not run on Linux, so the Tool project itself was not built end-to-end here).
• No automated tests exist in the repo (confirmed in .ai/repository-analysis.instructions.md); manual verification against a live IMAP server (e.g. imap.gmail.com:143) is the only available path — reviewer should run Tool/Common/Test_SSL_Support.bat or the GUI scan against a real STARTTLS-on-143 server and confirm the certificate fields populate.

Symbiosis

Parent of any future IMAP-related improvements (none filed). No other open rows depend on this fix.

🤖 Generated with Claude Code

[EvoJocys]

---

🤖 AI-Assisted Review
Created by AI Platform agent with PR Review

---

[AI Review]

🟢 Risk: Low — single contained protocol fix, no security/data impact
🟢 Confidence: High — diff fully reviewed against RFC 3501/2595

Decision

✅ Approve with comments

Why: Correctly replaces POP3 wire format on the IMAP branch with RFC-conformant greeting and tagged STARTTLS; only minor tightening of the success check is suggested.

Related

• TECH: SslScanner Improvement — Fix IMAP STARTTLS protocol in Test_SSL_Support #8 — IMAP STARTTLS protocol error

Blockers

• (None.)

Suggestions

Suggestion 1 · 🟡 Recommended

File: Tool/Common/Test_SSL_Support.cs
Line range: 284-286

Reason: IndexOf(" OK", StringComparison.OrdinalIgnoreCase) is a substring search, so a tagged failure like a001 NO server temporarily OK-ish, retry later would still satisfy the check and the code would then call AuthenticateAsClient on a server that never enabled TLS. Anchor the check on the tag we sent (a001 OK) to guarantee we matched the success response for this command.

					// STARTTLS (RFC 2595 — tagged command)
					clearTextWriter.WriteLine("a001 STARTTLS");
					var startTlsResponse = clearTextReader.ReadLine();
					if (startTlsResponse == null || !startTlsResponse.StartsWith("a001 OK", StringComparison.OrdinalIgnoreCase))
						throw new InvalidOperationException("IMAP Server did not respond to STARTTLS request");

Suggestion 2 · ⚪ Optional

File: Tool/Common/Test_SSL_Support.cs
Line range: 279-281

Reason: An IMAP server may legitimately greet with * PREAUTH (already authenticated) or refuse with * BYE. The current StartsWith("* OK") is correct for the success case, but a null guard plus a slightly more permissive matcher would mirror the more defensive POP3/SMTP branches and avoid NullReferenceException if the server closes the socket without a reply.

					var connectResponse = clearTextReader.ReadLine();
					if (connectResponse == null || !connectResponse.StartsWith("* OK", StringComparison.OrdinalIgnoreCase))
						throw new InvalidOperationException("IMAP Server did not respond to connection request");

Suggestion 3 · ⚪ Optional

File: Tool/Common/Test_SSL_Support.cs
Line range: 277-290

Reason: Many real IMAP servers emit untagged status lines (* OK …, * CAPABILITY …) before the tagged STARTTLS reply. ReadLine() only consumes one line, so on those servers the check now sees the untagged line and still passes (because it contains " OK"), but the real tagged response is left in the buffer for AuthenticateAsClient to choke on. Loop until a line starting with a001 arrives. Not blocking because the common case (single tagged response) already works.

					// STARTTLS (RFC 2595 — tagged command)
					clearTextWriter.WriteLine("a001 STARTTLS");
					string startTlsResponse;
					do { startTlsResponse = clearTextReader.ReadLine(); }
					while (startTlsResponse != null && !startTlsResponse.StartsWith("a001 ", StringComparison.OrdinalIgnoreCase));
					if (startTlsResponse == null || !startTlsResponse.StartsWith("a001 OK", StringComparison.OrdinalIgnoreCase))
						throw new InvalidOperationException("IMAP Server did not respond to STARTTLS request");

How it works

The IMAP branch in Test_SSL_Support.TryStartTls (port 143) was a copy of the POP3 wire protocol: it expected a +OK greeting and sent the bare STLS command. IMAP, however, uses * OK … as its untagged greeting (RFC 3501) and requires tagged commands for STARTTLS (RFC 2595), so every scan of an IMAP-with-STARTTLS endpoint threw InvalidOperationException before TLS negotiation could start.

This PR replaces those two checks: the greeting test becomes StartsWith("* OK") and the command becomes the tagged a001 STARTTLS. The success matcher is relaxed to a case-insensitive " OK" substring so a tagged a001 OK … reply (in any case) is accepted. The changelog and <Version> in JocysCom.SslScanner.Tool.csproj are bumped to 1.1.7, and the tracking file .ai/work-item-improvement.md moves WI-1 from Working to Done.

sequenceDiagram
    participant S as Scanner
    participant I as IMAP server
    S->>I: TCP connect :143
    I-->>S: * OK IMAP4rev1 ready
    S->>I: a001 STARTTLS
    I-->>S: a001 OK Begin TLS now
    S->>I: TLS handshake (AuthenticateAsClient)

Loading

How to test

1. Build the solution to confirm the change compiles:
   dotnet build Tool/JocysCom.SslScanner.Tool.csproj /v:minimal
2. Run the tool against a known IMAP STARTTLS host (any major mail provider; e.g. imap.gmail.com port 143) and verify the scan now completes with TLS metadata instead of throwing IMAP Server did not respond to connection request / ... STLS request.
3. (Negative) point the tool at a port that is not IMAP (e.g. force port 143 against a POP3 daemon) and confirm the error message now says STARTTLS request (not STLS request).
4. Confirm POP3 (port 110) and SMTP (port 25) scans still succeed — those branches were not touched, but the IMAP block sits between them.

Files to review first

• Tool/Common/Test_SSL_Support.cs — the actual protocol fix; all behaviour change is here.
• Tool/JocysCom.SslScanner.Tool.csproj — version bump must match the changelog.
• Tool/Documents/ChangeLog.txt — user-facing description of the fix.

Audit Details...

Context

• PR: https://github.com/JocysCom/SslScanner/_git/SslScanner/pullrequest/10
• Work Item: https://github.com/JocysCom/SslScanner/_workitems/edit/8
• Repo: SslScanner (SslScanner)
• Base branch: main
• Feature branch: feat/wi-8-tech-sslscanner-improvement-fix-imap-sta

Scope

• Files changed count: 4 (1 source, 1 project file, 1 changelog, 1 internal tracking doc)
• Key areas: Tool/Common/Test_SSL_Support.cs — the IMAP port-143 branch inside TryStartTls
• Notable additions: tagged a001 STARTTLS command; case-insensitive success match; RFC 3501 reference in the inline comment
• Notable removals: POP3-style +OK greeting check and bare STLS command on the IMAP path
• Renames and migrations: none

Strengths

• Surgical fix: only the broken IMAP branch is touched; POP3 (110) and SMTP (25) branches are untouched, so existing protocols cannot regress
• Inline comment now cites both RFCs (3501 for greeting, 2595 for STARTTLS) which is the right pointer for future maintainers
• Changelog and project <Version> are bumped together (1.1.6 → 1.1.7), keeping the user-visible artefacts consistent
• Internal tracking file (work-item-improvement.md) is updated to reflect the work-item status transition, keeping the project plan in sync with the code change

Issues and risks

• Loose success matcher: IndexOf(" OK", OrdinalIgnoreCase) < 0 is a substring scan rather than an anchored tag match. A tagged a001 NO …OK… (or any line containing " OK") would currently be accepted. See Suggestion 1.
• Single-line read assumes the server replies with exactly one line after a001 STARTTLS. RFC-compliant servers may emit untagged * … lines first; the current check would consume the wrong line. Practical impact is low because most production IMAP servers respond directly with the tagged line. See Suggestion 3.
• Null-dereference: ReadLine() can return null if the peer closes the connection. The new IMAP block keeps the existing POP3/SMTP pattern (would NRE), so this is a pre-existing project-wide issue, not a regression. Flagged in Suggestion 2.
• No regression test added (see Testing).

Testing

• Existing tests affected: none — no test project exists in the repository.
• New tests added: none.
• Coverage adequacy for risk level: low. The bug being fixed (entire IMAP STARTTLS path throwing) is exactly the kind of regression a smoke test would have caught.
• Build / validation evidence: build NOT executed in this review (best-effort sanity check only); the diff is small and self-contained, and the change site has no compile-time coupling beyond System.StringComparison (already imported transitively in the file).
• Manual validation steps required: see "How to test" above.
• Recommended durable tests:
  • Unit (preferred): a small test that uses NetworkStream-style readers/writers wrapped over an in-memory Stream pair to drive a fake IMAP server (* OK …\r\na001 OK Begin TLS now\r\n) and asserts that the STARTTLS exchange completes without throwing. Likely path under the canonical mirror pattern: Tests/Tool.Tests/Common/Test_SSL_SupportTests.cs.
  • Integration (optional): a [Fact(Skip="manual")] test that connects to a known public IMAP STARTTLS endpoint and asserts a non-null result. Same target file.

Security

• Secrets and credentials posture: unchanged. No credentials are read or transmitted on the cleartext side; only the protocol handshake.
• Input validation and output encoding: see "Issues and risks" — the loose success matcher is the only validation concern, and it does not expand attack surface.
• Authorization checks at boundaries: n/a (read-only scanner).
• Dependency changes and known CVEs: none — no PackageReference or runtime changes.

Performance

• Hot paths affected: none. The IMAP path runs once per scanned host, so a one-line ReadLine round-trip change is irrelevant to throughput.
• Complexity and algorithmic considerations: O(1) per host.
• I/O and data access patterns: unchanged — same single connect, single ReadLine, single WriteLine, single ReadLine sequence.
• Caching or batching opportunities: n/a.

Operations

• Migrations and reversibility: pure code change, instantly reversible by reverting.
• Configuration and feature flags: none introduced.
• Logging/metrics/tracing changes: none. The thrown InvalidOperationException message is updated from "STLS request" to "STARTTLS request"; any downstream consumer matching on the old string would see a behavioural shift, but no in-repo consumer matches on it.
• Rollout plan or canary considerations: n/a for a desktop scanning tool; ship via normal version bump.

Documentation

• README or service docs updates required: none.
• Changelog entry: present and accurate (2026-05-26 v1.1.7 - Fix: IMAP STARTTLS protocol …).
• Inline code comments quality: improved — comment now points to both relevant RFCs and clarifies that the command is tagged.

Diff overview

• See .tmp/pr-review/all-pre-content.txt, all-post-content.txt, all-diffs.txt.