fix(import): harden WinSCP password and site import reliability

src/portkeydrop/dialogs/import_connections.py

@@ -278,17 +278,29 @@ def _load_preview(self) -> bool:
         try:
             self._loaded_sites = load_from_source(source, path)
         except Exception as exc:
+            message = f"Failed to parse configuration: {exc}"
+            if source == "winscp":
+                message += (
+                    "\n\nTip: WinSCP imports support INI exports and Windows Registry sessions. "
+                    "Passwords protected by WinSCP master password cannot be decrypted."
+                )
             wx.MessageBox(
-                f"Failed to parse configuration: {exc}",
+                message,
                 "Import Sites",
                 wx.OK | wx.ICON_ERROR,
                 self,
             )
             return False
 
         if not self._loaded_sites:
+            message = "No connections were found in the selected source."
+            if source == "winscp":
+                message += (
+                    "\n\nTip: Make sure the file contains [Sessions\\...] entries, "
+                    "or use Auto-Detect to load sessions from the Windows Registry."
+                )
             wx.MessageBox(
-                "No connections were found in the selected source.",
+                message,
                 "Import Sites",
                 wx.OK | wx.ICON_INFORMATION,
                 self,

src/portkeydrop/importers/winscp.py

@@ -3,6 +3,7 @@
 from __future__ import annotations
 
 import configparser
+import locale
 import logging
 import os
 import sys
@@ -14,6 +15,10 @@
 logger = logging.getLogger(__name__)
 
 _MAGIC = 0xA3
+_PWALG_SIMPLE_FLAG = 0xFF
+_PWALG_SIMPLE_INTERNAL = 0x00
+_PWALG_SIMPLE_EXTERNAL = 0x01
+_PWALG_SIMPLE_INTERNAL2 = 0x02
 
 _NUMERIC_PROTOCOL_MAP = {
     "0": "sftp",
@@ -33,45 +38,18 @@ def detect_ini_path() -> Path:
 
 def parse_ini_file(path: Path) -> list[ImportedSite]:
     """Parse WinSCP exported INI file."""
-    parser = configparser.RawConfigParser(interpolation=None)
+    parser = configparser.RawConfigParser(interpolation=None, strict=False)
     parser.optionxform = str
-    parser.read(path, encoding="utf-8")
+    parser.read_string(_read_ini_text(path), source=str(path))
 
     sites: list[ImportedSite] = []
     for section in parser.sections():
         if not section.startswith("Sessions\\"):
             continue
 
-        cfg = parser[section]
-        host = cfg.get("HostName", "").strip()
-        if not host:
-            continue
-
-        raw_port = cfg.get("PortNumber", "").strip()
-        port = int(raw_port) if raw_port.isdigit() else 0
-
-        protocol = _detect_protocol(cfg)
-        if protocol == "scp":
-            protocol = "sftp"
-
-        username = cfg.get("UserName", "").strip()
-        initial_dir = cfg.get("RemoteDirectory", "").strip() or "/"
-        key_path = cfg.get("PublicKeyFile", "").strip()
-        name = _decode_name(section.removeprefix("Sessions\\"))
-        password = _safe_decrypt(cfg.get("Password", "").strip(), username, host)
-
-        sites.append(
-            ImportedSite(
-                name=name,
-                protocol=protocol,
-                host=host,
-                port=port,
-                username=username,
-                password=password,
-                key_path=key_path,
-                initial_dir=initial_dir,
-            )
-        )
+        parsed = _parse_site(parser[section], section.removeprefix("Sessions\\"))
+        if parsed is not None:
+            sites.append(parsed)
     return sites
 
 
@@ -100,40 +78,70 @@ def parse_registry_sessions() -> list[ImportedSite]:
 
                 with winreg.OpenKey(sessions_key, session_name) as session_key:
                     values = _read_reg_values(winreg, session_key)
-                    host = values.get("HostName", "").strip()
-                    if not host:
-                        continue
-
-                    raw_port = values.get("PortNumber", "").strip()
-                    port = int(raw_port) if raw_port.isdigit() else 0
-
-                    protocol = _detect_protocol(values)
-                    if protocol == "scp":
-                        protocol = "sftp"
-
-                    username = values.get("UserName", "").strip()
-                    initial_dir = values.get("RemoteDirectory", "").strip() or "/"
-                    key_path_value = values.get("PublicKeyFile", "").strip()
-                    password = _safe_decrypt(values.get("Password", "").strip(), username, host)
-
-                    sites.append(
-                        ImportedSite(
-                            name=_decode_name(session_name),
-                            protocol=protocol,
-                            host=host,
-                            port=port,
-                            username=username,
-                            password=password,
-                            key_path=key_path_value,
-                            initial_dir=initial_dir,
-                        )
-                    )
+                    parsed = _parse_site(values, session_name)
+                    if parsed is not None:
+                        sites.append(parsed)
     except OSError:
         return []
 
     return sites
 
 
+def _read_ini_text(path: Path) -> str:
+    data = path.read_bytes()
+    tried: set[str] = set()
+    encodings = [
+        "utf-8-sig",
+        "utf-16",
+        "utf-16-le",
+        "utf-16-be",
+        locale.getpreferredencoding(False),
+        "cp1252",
+        "latin-1",
+    ]
+    for encoding in encodings:
+        if not encoding or encoding in tried:
+            continue
+        tried.add(encoding)
+        try:
+            return data.decode(encoding)
+        except UnicodeDecodeError:
+            continue
+    return data.decode("latin-1")
+
+
+def _parse_site(
+    cfg: configparser.SectionProxy | dict[str, str], raw_name: str
+) -> ImportedSite | None:
+    host = _decode_value(str(cfg.get("HostName", "")).strip())
+    if not host:
+        return None
+
+    raw_port = str(cfg.get("PortNumber", "")).strip()
+    port = int(raw_port) if raw_port.isdigit() else 0
+
+    protocol = _detect_protocol(cfg)
+    if protocol == "scp":
+        protocol = "sftp"
+
+    username = str(cfg.get("UserName", "")).strip()
+    initial_dir = _decode_value(str(cfg.get("RemoteDirectory", "")).strip()) or "/"
+    key_path = _decode_value(str(cfg.get("PublicKeyFile", "")).strip())
+    name = _decode_name(raw_name)
+    password = _safe_decrypt(str(cfg.get("Password", "")).strip(), username, host)
+
+    return ImportedSite(
+        name=name,
+        protocol=protocol,
+        host=host,
+        port=port,
+        username=username,
+        password=password,
+        key_path=key_path,
+        initial_dir=initial_dir,
+    )
+
+
 def _read_reg_values(winreg, key) -> dict[str, str]:
     values: dict[str, str] = {}
     idx = 0
@@ -163,7 +171,13 @@ def _detect_protocol(cfg: configparser.SectionProxy | dict[str, str]) -> str:
 
 
 def _decode_name(raw_name: str) -> str:
-    return unquote(raw_name).replace("%5C", "\\")
+    return _decode_value(raw_name).replace("%5C", "\\")
+
+
+def _decode_value(value: str) -> str:
+    if not value:
+        return ""
+    return unquote(value)
 
 
 def _safe_decrypt(encrypted: str, username: str, hostname: str) -> str:
@@ -180,29 +194,47 @@ def _safe_decrypt(encrypted: str, username: str, hostname: str) -> str:
 def _decrypt_winscp_password(username: str, hostname: str, encrypted: str) -> str:
     """Decrypt a WinSCP XOR-obfuscated password.
 
-    Algorithm reference: https://github.com/NetSPI/WinSCPPasswordDecryptor
+    Algorithm reference: WinSCP source `source/core/Security.cpp`
     """
-    key = username + hostname
+    if len(encrypted) % 2 != 0:
+        raise ValueError("Encrypted WinSCP password must be even-length hex")
     encrypted_hex = [encrypted[i : i + 2] for i in range(0, len(encrypted), 2)]
 
     flag = _decrypt_next(encrypted_hex)
-    if flag == 0xFF:
-        _decrypt_next(encrypted_hex)  # skip byte
-        length = _decrypt_next(encrypted_hex)
+    if flag == _PWALG_SIMPLE_FLAG:
+        version = _decrypt_next(encrypted_hex)
+        if version == _PWALG_SIMPLE_INTERNAL:
+            length = _decrypt_next(encrypted_hex)
+        elif version == _PWALG_SIMPLE_INTERNAL2:
+            length = (_decrypt_next(encrypted_hex) << 8) + _decrypt_next(encrypted_hex)
+        elif version == _PWALG_SIMPLE_EXTERNAL:
+            raise ValueError("WinSCP external/master-password encrypted value is unsupported")
+        else:
+            raise ValueError(f"Unsupported WinSCP password version: {version}")
     else:
         length = flag
 
-    result: list[str] = []
+    shift = _decrypt_next(encrypted_hex)
+    for _ in range(shift):
+        _decrypt_next(encrypted_hex)
+
+    result: list[int] = []
     for _ in range(length):
-        result.append(chr(_decrypt_next(encrypted_hex)))
+        result.append(_decrypt_next(encrypted_hex))
+
+    password_bytes = bytes(result)
+    if flag == _PWALG_SIMPLE_FLAG:
+        key = (username + hostname).encode("utf-8")
+        if not password_bytes.startswith(key):
+            raise ValueError("Decrypted WinSCP payload key-prefix mismatch")
+        password_bytes = password_bytes[len(key) :]
 
-    password = "".join(result)
-    if flag == 0xFF:
-        password = password[len(key) :]
-    return password
+    return password_bytes.decode("utf-8")
 
 
 def _decrypt_next(encrypted_hex: list[str]) -> int:
     """Consume one hex-encoded byte and return one decrypted byte."""
+    if not encrypted_hex:
+        raise ValueError("Unexpected end of encrypted WinSCP payload")
     b = int(encrypted_hex.pop(0), 16)
     return (~(b ^ _MAGIC)) & 0xFF

tests/fixtures/importers/winscp_real_export_sanitized.ini

@@ -0,0 +1,29 @@
+[Configuration\CDCache]
+ops@sftp-edge.example.net=412F666F6F
+
+[Configuration\Interface]
+QueueRememberPassword=1
+PuttyPassword=0
+
+[Configuration\Security]
+UseMasterPassword=0
+
+[Sessions\ops@sftp-edge.example.net]
+HostName=sftp-edge.example.net
+UserName=ops
+LocalDirectory=C:%5CUsers%5Csanitized%5CDocuments
+RemoteDirectory=/home/ops
+Password=__ENC_PASS_1__
+
+[Sessions\ops@legacy-name.example.net]
+HostName=sftp-backend.internal.example.net
+UserName=ops
+PublicKeyFile=C:%5CUsers%5Csanitized%5CKeys%5Cid_ed25519%20prod.ppk
+RemoteDirectory=/.ssh
+Password=__ENC_PASS_2__
+
+[Sessions\readonly@no-password.example.net]
+HostName=no-password.example.net
+UserName=readonly
+LocalDirectory=C:%5CUsers%5Csanitized%5CDocuments
+RemoteDirectory=/archive

tests/test_import_connections_dialog.py

@@ -309,3 +309,69 @@ def test_autodetect_uses_filtered_source_list(self, monkeypatch):
 
         mock_detect.assert_called_once_with("winscp")
         assert dlg.path_text.GetValue() == "/fake/WinSCP.ini"
+
+
+class TestWinSCPMessaging:
+    """WinSCP errors should include actionable troubleshooting guidance."""
+
+    def test_winscp_no_sites_message_includes_tip(self, monkeypatch):
+        DialogCls = _load_dialog(monkeypatch)
+        from portkeydrop.importers import ImportSource
+
+        with patch(
+            "portkeydrop.dialogs.import_connections.available_sources",
+            return_value=[
+                ImportSource("filezilla", "FileZilla"),
+                ImportSource("winscp", "WinSCP"),
+                ImportSource("from_file", "From file..."),
+            ],
+        ):
+            dlg = DialogCls(None)
+
+        dlg.source_radio.SetSelection(1)  # WinSCP
+        dlg.path_text.SetValue("")
+
+        with (
+            patch(
+                "portkeydrop.dialogs.import_connections.load_from_source",
+                return_value=[],
+            ),
+            patch("portkeydrop.dialogs.import_connections.wx.MessageBox") as mock_message,
+        ):
+            result = dlg._load_preview()
+
+        assert result is False
+        message = mock_message.call_args.args[0]
+        assert "No connections were found" in message
+        assert "[Sessions\\...]" in message
+
+    def test_winscp_parse_error_message_mentions_master_password(self, monkeypatch):
+        DialogCls = _load_dialog(monkeypatch)
+        from portkeydrop.importers import ImportSource
+
+        with patch(
+            "portkeydrop.dialogs.import_connections.available_sources",
+            return_value=[
+                ImportSource("filezilla", "FileZilla"),
+                ImportSource("winscp", "WinSCP"),
+                ImportSource("from_file", "From file..."),
+            ],
+        ):
+            dlg = DialogCls(None)
+
+        dlg.source_radio.SetSelection(1)  # WinSCP
+        dlg.path_text.SetValue("")
+
+        with (
+            patch(
+                "portkeydrop.dialogs.import_connections.load_from_source",
+                side_effect=ValueError("bad format"),
+            ),
+            patch("portkeydrop.dialogs.import_connections.wx.MessageBox") as mock_message,
+        ):
+            result = dlg._load_preview()
+
+        assert result is False
+        message = mock_message.call_args.args[0]
+        assert "Failed to parse configuration" in message
+        assert "master password" in message.lower()