From 241e35d3b6f4b2dfe2bfc90e65ef7fb9332716cf Mon Sep 17 00:00:00 2001 From: saphid <4596216+saphid@users.noreply.github.com> Date: Thu, 8 Oct 2026 19:32:34 +1100 Subject: [PATCH] Reports: no custom alias, banner-only ssh version, whole user@host fields Round-2 review (pr79-review-r2): - The summary names the alias only as default ("frame") or custom, and the ~/.ssh/config line says "for the active alias" without naming it. - ssh -V is parsed only as a banner at the start (OpenSSH_N.N[pN], optional LibreSSL/OpenSSL N.N.N) and rebuilt from fixed names and numbers; anything else is "unknown". - scrub: ssh's whole user@host field (spaces, DOMAIN\ prefix, full domain names, "user@host's password:") goes before the email rule; a home folder's whole name runs to the next separator (apostrophes too), prefixes matched whatever their case. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/privacy.md | 26 ++++++++++++++---------- tests/test_link.py | 47 ++++++++++++++++++++++++++++++++----------- ui/frame_report.py | 34 +++++++++++++++++++++++-------- ui/frame_telemetry.py | 15 +++++++------- ui/index.html | 2 +- 5 files changed, 85 insertions(+), 39 deletions(-) diff --git a/docs/privacy.md b/docs/privacy.md index 9a885dc..5bd7701 100644 --- a/docs/privacy.md +++ b/docs/privacy.md @@ -123,14 +123,14 @@ With **Include diagnostics** ticked (the default), the report adds: - the Frame's SteamOS build, if it has connected since the app started - which analytics levels are on - a short connection summary, so "the app can't find the headset" can be - diagnosed. It is made of fixed words, counts and the alias name only; no - text from an error message or from ssh goes in it: + diagnosed. It is made of fixed words and counts only; no text from an + error message or from ssh, and no name you chose, goes in it: - the connector's state (idle, connecting, connected or failed), the stage it failed at (network, find, ssh, identity or login), the attempt number and why it started (such as "Starting up" or "Trying again") - - how many headsets are saved, the active one's ssh alias name (such as - `frame`; it's whatever name the headset was set up under), whether it's - saved or a bare ssh alias, and its number of addresses + - how many headsets are saved; whether the active one's ssh alias is the + default `frame` or a custom one (a custom alias itself is never named); + whether it's saved or a bare ssh alias; and its number of addresses - for the current error and the last failure: the stage and an error category (the same fixed names as error details, such as `frame_unreachable`, `frame_not_set_up`, `frame_auth` or `other`), and @@ -146,14 +146,17 @@ With **Include diagnostics** ticked (the default), the report adds: off or not installed - which kind of `ssh` the app runs (Windows OpenSSH in System32, OpenSSH in Program Files, Git for Windows, MSYS2/Cygwin, Homebrew or /usr/local, - Nix, the system one, or "other"), never its path; its version words from - `ssh -V` (such as `OpenSSH_for_Windows_9.5p1, LibreSSL 3.8.2`, and - nothing else it prints); and the kinds of any other `ssh` on the PATH. + Nix, the system one, or "other"), never its path; its version, rebuilt + from the numbers in the banner `ssh -V` prints (such as + `OpenSSH for Windows 9.5p1, LibreSSL 3.8.2`; anything that isn't a + plain OpenSSH banner is reported as `unknown`); and the kinds of any + other `ssh` on the PATH. This is looked up once in the background after the app starts, so a report sent straight away may say "still being checked" - whether `~/.ssh/config` exists, whether it has Set Up Connection's managed block for the active alias, how many managed blocks it has, and - whether a hand-written `Host` line also names the alias + whether a hand-written `Host` line also names the alias (yes or no; the + alias isn't named) **Also include recent activity and the server log** is off by default, because those lines can name files and apps. When ticked, it adds the newest @@ -163,8 +166,9 @@ shown on the connection pill, ssh's last line about it, and the address kinds above. That free text is scrubbed when the line is written: the addresses and names of the headset being tried (whatever their case), and any name ssh gives after "hostname", "host" or "to", become ``; a Windows home folder's -whole name and the whole user part of `user@host` (spaces and `DOMAIN\user` -included) become ``; then the scrubbing below. A host name ssh mentions +whole name (spaces and apostrophes included) becomes ``, and ssh's +whole `user@host:` field (spaces, `DOMAIN\user` and full domain names +included) becomes `@:`; then the scrubbing below. A host name ssh mentions in some other wording can still get through, so check the log lines in **Show exactly what's included** before sending. A failure that repeats on every retry is written at most once every 5 minutes. diff --git a/tests/test_link.py b/tests/test_link.py index 4568201..626fe00 100644 --- a/tests/test_link.py +++ b/tests/test_link.py @@ -610,7 +610,7 @@ class Connecting(unittest.TestCase): done.start() done.join() with mock.patch.object(fr, "link", self.link), \ - mock.patch.dict(fr._ssh, {"thread": done, "line": "SSH: system OpenSSH, OpenSSH_9.9p1, LibreSSL 3.3.6"}): + mock.patch.dict(fr._ssh, {"thread": done, "line": "SSH: system OpenSSH, OpenSSH 9.9p1, LibreSSL 3.3.6"}): return fr.diagnostics() def test_each_failure_goes_to_the_server_log_scrubbed(self): @@ -634,15 +634,17 @@ class Connecting(unittest.TestCase): self.link.connect(["start"]) text = self.report() self.assertIn("Connection: failed at find, attempt 1 (Starting up)", text) - self.assertIn('Headsets: 1 saved; active "frame-t" (saved, 2 address(es))', text) + self.assertIn("Headsets: 1 saved; active alias custom (saved, 2 address(es))", text) self.assertIn("Error: find, frame_not_set_up", text) self.assertRegex(text, r"Last failure: find, frame_not_set_up, 0 min \d+ s ago") self.assertIn("Addresses tried: hostname unresolved; hostname unresolved", text) self.assertIn("Network: gateway yes, Tailscale not installed", text) - self.assertIn("SSH: system OpenSSH, OpenSSH_9.9p1", text) - self.assertIn('~/.ssh/config: managed "frame-t" block yes (1 managed in all); hand-written "Host frame-t" yes', text) - # No free text from the error or ssh at all. - for leaked in ("steamdeck-jane", "Could not resolve", "Can't find", "192.168", "10.0.0.7", "steamos", str(self.dir)): + self.assertIn("SSH: system OpenSSH, OpenSSH 9.9p1", text) + self.assertIn("~/.ssh/config: managed block for the active alias yes (1 managed in all); " + "hand-written Host for it yes", text) + # No free text from the error or ssh at all, and not the custom alias. + for leaked in ("steamdeck-jane", "Could not resolve", "Can't find", "192.168", "10.0.0.7", "steamos", + "frame-t", str(self.dir)): self.assertNotIn(leaked, text) def test_a_failure_on_the_last_headset_leaves_nothing_of_it_after_switching(self): @@ -716,10 +718,21 @@ class FailureLog(unittest.TestCase): fl.log_failure("login", "refused", r"debug | ssh said: CORP\jane@frame's password: denied") out = err.getvalue() self.assertIn(r"C:\Users\/.ssh/config", out) - self.assertIn("@frame: Permission denied", out) + self.assertIn("@: Permission denied", out) for leaked in ("Jane", "Doe", "jane", "CORP"): self.assertNotIn(leaked, out) + def test_whole_ssh_user_at_host_fields_and_home_folders_go(self): + import frame_telemetry as tm + with mock.patch.object(tm, "_user_names", return_value=set()): + self.assertEqual(tm.scrub(r"CORP\Jane Doe@jane-office.example.com: Permission denied (publickey)."), + "@: Permission denied (publickey).") + self.assertEqual(tm.scrub("jane@jane-office.example.com's password: "), "@'s password: ") + self.assertEqual(tm.scrub(r"Bad owner on C:\Users\O'Brien/.ssh/config"), r"Bad owner on C:\Users\/.ssh/config") + self.assertEqual(tm.scrub(r"c:\users\Zoë Smith.Jr\.ssh\config"), r"c:\users\\.ssh\config") + self.assertEqual(tm.scrub("/users/O'Brien/x and /HOME/jane doe/y"), "/users//x and /HOME//y") + self.assertEqual(tm.scrub("write to me@example.com today"), "write to today") + class ConnectionDiagnostics(unittest.TestCase): def test_address_kinds_never_the_address(self): @@ -743,13 +756,23 @@ class ConnectionDiagnostics(unittest.TestCase): self.assertEqual(fl.hide_hosts("reset by SteamDeck", ["steamdeck"]), "reset by ") self.assertEqual(fl.hide_operands("The headset took too long to answer."), "The headset took too long to answer.") - def test_ssh_version_is_only_its_version_words(self): + def test_ssh_version_is_rebuilt_from_its_numbers(self): import frame_report as fr - for said, want in (("OpenSSH_for_Windows_9.5p1, LibreSSL 3.8.2\n", "OpenSSH_for_Windows_9.5p1, LibreSSL 3.8.2"), - ("OpenSSH_9.9p1, LibreSSL 3.3.6\n", "OpenSSH_9.9p1, LibreSSL 3.3.6"), - (r"C:\Users\Jane Doe\ssh.exe: something odd", "unrecognised version (exit 0)")): + for said, want in (("OpenSSH_for_Windows_9.5p1, LibreSSL 3.8.2\n", "OpenSSH for Windows 9.5p1, LibreSSL 3.8.2"), + ("OpenSSH_9.9p1, LibreSSL 3.3.6\n", "OpenSSH 9.9p1, LibreSSL 3.3.6"), + ("OpenSSH_8.9p1 Ubuntu-3ubuntu0.10, OpenSSL 3.0.2 15 Mar 2022\n", "OpenSSH 8.9p1"), + ("OpenSSH_9.6p1, OpenSSL 3.0.13 30 Jan 2024\n", "OpenSSH 9.6p1, OpenSSL 3.0.13"), + ("OpenSSH_9.9p1-Jane-Doe-Laptop, LibreSSL 3.3.6\n", "unknown"), + ("OpenSSH_9.9p1, OpenSSL jane-laptop\n", "OpenSSH 9.9p1"), + (r"C:\Users\Jane\OpenSSH-portable\ssh.exe: not found", "unknown"), + ("", "unknown")): with mock.patch.object(fr.frame_host, "run_ssh", return_value=subprocess.CompletedProcess([], 0, "", said)): - self.assertEqual(fr.ssh_version("ssh"), want) + self.assertEqual(fr.ssh_version("ssh"), want, said) + + def test_only_the_default_alias_is_named(self): + import frame_report as fr + self.assertEqual(fr.alias_kind("frame"), 'default ("frame")') + self.assertEqual(fr.alias_kind("jane-office.example.com"), "custom") def test_a_slow_path_never_holds_up_a_report(self): import frame_report as fr diff --git a/ui/frame_report.py b/ui/frame_report.py index 75a0f77..93aae3e 100644 --- a/ui/frame_report.py +++ b/ui/frame_report.py @@ -98,18 +98,31 @@ def _ssh_on_path(): return found -VERSION_RE = re.compile(r"\b((?:OpenSSH|Sun_SSH|dropbear)[\w.\-]*)(?:[, ]+((?:LibreSSL|OpenSSL) [\w.\-]+))?", re.I) +# Only a real banner at the very start ("OpenSSH_9.9p1, LibreSSL 3.3.6", +# "OpenSSH_for_Windows_9.5p1, LibreSSL 3.8.2"): rebuilt from fixed names and its numbers. +BANNER_RE = re.compile(r"OpenSSH_(for_Windows_)?(\d+)\.(\d+)(p\d+)?(?=[,\s]|$)") +LIBRARY_RE = re.compile(r",?\s*(LibreSSL|OpenSSL) (\d+\.\d+\.\d+[a-z]?)(?=[,\s]|$)") + + +def parse_ssh_version(said): + """"OpenSSH 9.9p1, LibreSSL 3.3.6"-style text from `ssh -V`'s output, or "unknown".""" + said = (said or "").lstrip() + m = BANNER_RE.match(said) + if not m: + return "unknown" + out = f"OpenSSH{' for Windows' if m.group(1) else ''} {m.group(2)}.{m.group(3)}{m.group(4) or ''}" + lib = LIBRARY_RE.match(said, m.end()) + return out + (f", {lib.group(1)} {lib.group(2)}" if lib else "") def ssh_version(path): - """The version words of `ssh -V` (it prints to stderr), nothing else it says.""" + """The version from `ssh -V` (it prints to stderr), nothing else it says.""" try: r = frame_host.run_ssh([path, "-V"], capture_output=True, stdin=subprocess.DEVNULL, text=True, errors="replace", timeout=5) except (OSError, subprocess.SubprocessError) as e: return f"couldn't run it ({type(e).__name__})" - m = VERSION_RE.search((r.stderr or "") + " " + (r.stdout or "")) - return ", ".join(g for g in m.groups() if g) if m else f"unrecognised version (exit {r.returncode})" + return parse_ssh_version(r.stderr or r.stdout) def _ssh_check(): @@ -168,13 +181,18 @@ def config_line(alias): outside.append(line) own = any(alias in re.split(r"[\s=]+", ln.strip())[1:] for ln in outside if re.match(r"(?i)\s*host[\s=]", ln)) - return (f"~/.ssh/config: managed \"{alias}\" block {'yes' if alias in blocks else 'no'} " - f"({len(blocks)} managed in all); hand-written \"Host {alias}\" {'yes' if own else 'no'}") + return (f"~/.ssh/config: managed block for the active alias {'yes' if alias in blocks else 'no'} " + f"({len(blocks)} managed in all); hand-written Host for it {'yes' if own else 'no'}") + + +def alias_kind(alias): + """`default ("frame")` or `custom`: a name someone chose can say who they are.""" + return 'default ("frame")' if alias == "frame" else "custom" def connection_lines(): """A short summary of the connector in fixed words only: states, stages, error categories, - kinds of address, counts. No text from errors or ssh, and never an address.""" + kinds of address, counts. No text from errors or ssh, no custom alias, never an address.""" if link is None: return ["Connection: no connector (this server doesn't reach a headset over SSH)"] snap = link.snapshot() @@ -191,7 +209,7 @@ def connection_lines(): lines = [head] kind = ("none set up" if active.get("none") else "a bare ssh alias" if active.get("transient") else f"saved, {len(active.get('addresses') or [])} address(es)") - lines.append(f"Headsets: {len(link.reg.devices())} saved; active \"{alias}\" ({kind})") + lines.append(f"Headsets: {len(link.reg.devices())} saved; active alias {alias_kind(alias)} ({kind})") if err: lines.append(f"Error: {err.get('stage')}, {frame_link.failure_category(err.get('message'), err.get('raw'))}") last = link.last_failure diff --git a/ui/frame_telemetry.py b/ui/frame_telemetry.py index 3c39d60..e2a071c 100644 --- a/ui/frame_telemetry.py +++ b/ui/frame_telemetry.py @@ -229,15 +229,16 @@ def scrub(text, limit=2000): home = str(Path.home()) if len(home) > 3: t = t.replace(home, '~') - t = re.sub(r'(/Users/|/home/)[^/\\\s]+', r'\1', t) - # A Windows home folder's whole name, spaces and all ("C:\Users\Jane Doe\..."), up to the next separator. - t = re.sub(r'''([A-Za-z]:[\\/]+Users[\\/]+)[^\\/\n'"]+''', r'\1', t) + # A home folder's whole name ("C:\Users\Jane Doe", "/Users/O'Brien"), up to the next separator. + t = re.sub(r'''(?i)(/Users/|/home/|[A-Za-z]:[\\/]+Users[\\/]+)[^\\/\n"]+''', r'\1', t) + # ssh's "user@host: ..." and "user@host's password:", the whole field: the user even with + # spaces or a DOMAIN\ prefix, the host even a full domain name. Before the email rule, which + # would otherwise take only the last word of the user. + t = re.sub(r'''(?m)(?:^|(?<=: )|(?<=\| ))[^@\n:|"<]{1,64}@[\w.\[\]%:<>-]+?(?=:(?:\s|$)|'s\s)''', + '@', t) for pattern, repl in SCRUBS: t = pattern.sub(repl, t) - # ssh's "user@host: ..." and "user@host's password": the whole user part, even "Jane Doe" or - # DOMAIN\user, when it starts a line or follows ": " or "| "; then any other word@host. - t = re.sub(r'''(?m)(?:^|(?<=: )|(?<=\| ))[^@\n:|'"<]{1,64}@(?=[\w.\[\]%<>-]+(?::|'s\s))''', '@', t) - t = re.sub(r'(?@', t) + t = re.sub(r'(?@', t) # any other word@host t = IPV6_RE.sub(_ipv6, t) for name in _user_names(): t = re.sub(r'\b%s\b' % re.escape(name), '', t) diff --git a/ui/index.html b/ui/index.html index 75cb027..02d0493 100644 --- a/ui/index.html +++ b/ui/index.html @@ -1272,7 +1272,7 @@ + Frame Control's version, your OS, the Frame's SteamOS build, and a connection summary in fixed words: how connecting went, the kinds of address tried, your ssh version and whether ~/.ssh/config has the headset's entry. No error text, no custom alias name, and never the addresses themselves.
Show exactly what's included
Loading…