mirror of
https://github.com/saphid/frame-control.git
synced 2026-10-29 04:02:36 +01:00
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) <noreply@anthropic.com>
This commit is contained in:
1 parent
70309d3cec
commit
241e35d3b6
5 files changed
+85
-39
No files matched your search
+15
-11
@@ -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 `<host>`; a Windows home folder's
|
||||
whole name and the whole user part of `user@host` (spaces and `DOMAIN\user`
|
||||
included) become `<user>`; then the scrubbing below. A host name ssh mentions
|
||||
whole name (spaces and apostrophes included) becomes `<user>`, and ssh's
|
||||
whole `user@host:` field (spaces, `DOMAIN\user` and full domain names
|
||||
included) becomes `<user>@<host>:`; 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.
|
||||
|
||||
+35
-12
@@ -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\<user>/.ssh/config", out)
|
||||
self.assertIn("<user>@frame: Permission denied", out)
|
||||
self.assertIn("<user>@<host>: 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)."),
|
||||
"<user>@<host>: Permission denied (publickey).")
|
||||
self.assertEqual(tm.scrub("jane@jane-office.example.com's password: "), "<user>@<host>'s password: ")
|
||||
self.assertEqual(tm.scrub(r"Bad owner on C:\Users\O'Brien/.ssh/config"), r"Bad owner on C:\Users\<user>/.ssh/config")
|
||||
self.assertEqual(tm.scrub(r"c:\users\Zoë Smith.Jr\.ssh\config"), r"c:\users\<user>\.ssh\config")
|
||||
self.assertEqual(tm.scrub("/users/O'Brien/x and /HOME/jane doe/y"), "/users/<user>/x and /HOME/<user>/y")
|
||||
self.assertEqual(tm.scrub("write to me@example.com today"), "write to <email> 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 <host>")
|
||||
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
|
||||
|
||||
+26
-8
@@ -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
|
||||
|
||||
@@ -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<user>', 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<user>', 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<user>', 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)''',
|
||||
'<user>@<host>', 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))''', '<user>@', t)
|
||||
t = re.sub(r'(?<![\w.+\\<-])[\w.+\\-]+@(?=[A-Za-z\[<])', '<user>@', t)
|
||||
t = re.sub(r'(?<![\w.+\\<-])[\w.+\\-]+@(?=[A-Za-z\[<])', '<user>@', 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), '<user>', t)
|
||||
|
||||
+1
-1
@@ -1272,7 +1272,7 @@
|
||||
<label class="field">Your email address<input type="email" id="bugContact" maxlength="254" placeholder="you@example.com" disabled></label>
|
||||
<p class="hint" id="bugReplaces" role="status" hidden></p>
|
||||
<label class="popt"><input type="checkbox" id="bugDiag" checked><b>Include diagnostics</b>
|
||||
<span class="sub">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, and never the addresses themselves.</span></label>
|
||||
<span class="sub">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.</span></label>
|
||||
<label class="popt"><input type="checkbox" id="bugLogs"><b>Also include recent activity and the server log</b>
|
||||
<span class="sub">Often shows what went wrong, but can contain file and app names. Check it below before sending.</span></label>
|
||||
<details id="bugDiagBox"><summary>Show exactly what's included</summary><div class="sentlog" id="bugDiagText">Loading…</div></details>
|
||||
|
||||
Reference in new issue
Block a user