Follow-up from a deep review of #73 (15de5c6) after pulling it into the
gorevds fork. The fix is correct and not exploitable — the vault key +
AAD binding and the SSH password stay the auth boundary, and authorize_target
still enforces allowed_users / denied_users / fixed-username. The items
below are follow-up suggestions, ranked.
1. Match on the connection name, not host:port
authorize_target reconstructs which prompt connection a saved card belongs to
by matching host:port. But the originating connection name is already on
the card — commitVaultSave stamps it (websh.js:2895,
saveEntry.connection = selectedPrompt.name). It just never reaches the
server: buildConnectBody's vault branch return b at websh.js:781 sits
before the if (rec.connection) b.connection = … line at :787, so
connection is dropped from the vault POST body.
Sending rec.connection and vetting via find_config_connection would be
unambiguous and rename-safe. host:port matching has two consequences:
- Ambiguity — two prompt connections on the same
host:port with
different allowed_users / denied_users: find_prompt_connection_by_host
returns the first, so the effective policy becomes file-order-dependent.
- Rename fragility — change the prompt connection's host or port and every
saved card silently breaks with the same "not allowed" this PR set out to
fix.
host:port would still be needed as a fallback for legacy cards saved before
connection-name tagging (the websh.js:2809 auto-match path) — so:
name-keyed primary, host:port as a documented legacy shim.
2. Scan-pattern detection skips vault rejections under restrict_hosts
Under restrict_hosts, _record_deny_for_scan is never called
(if not cfg["restrict_hosts"]). That was fine when a manual POST there was
just a stale UI — but a saved-card POST is now a real accept/reject surface,
so host/slot probing under restrict_hosts accumulates deny_blocked and
never scan_pattern, evading a fail2ban rule keyed on that signal. Feeding the
scanner for is_saved rejections would close it.
3. No end-to-end test of the handler wiring
All 7 new tests call authorize_target() directly. The handler path the PR
actually ships — is_saved derivation, the call site, host/port
resolved-from-vault — has no coverage. An integration test through
/api/connect would pin it. Also untested: ready-connection-not-matched
(the function's core claim), port mismatch, and restrict-off + deny-listed
saved card.
4. Smaller items
- Host compare is case-sensitive —
c.get("host","") == host.
_parse_denied_hosts already lowercases; a card whose host casing differs
from websh.json is falsely rejected. Casefolding both sides would match the
existing convention.
restrict_hosts docs now predate the change — the "raw POSTs rejected /
only via a named connection" wording no longer holds for the vault shape.
- Tests assert
assertIn("not allowed", err) — that substring matches two
distinct reject reasons (and not the third), so a wrong-reason rejection
passes; exact assertEqual would be tighter.
Happy to send a PR for the name-keyed approach + the cheap fixes if useful.
Follow-up from a deep review of #73 (
15de5c6) after pulling it into thegorevdsfork. The fix is correct and not exploitable — the vault key +AAD binding and the SSH password stay the auth boundary, and
authorize_targetstill enforces
allowed_users/denied_users/ fixed-username. The itemsbelow are follow-up suggestions, ranked.
1. Match on the connection name, not
host:portauthorize_targetreconstructs which prompt connection a saved card belongs toby matching
host:port. But the originating connection name is already onthe card —
commitVaultSavestamps it (websh.js:2895,saveEntry.connection = selectedPrompt.name). It just never reaches theserver:
buildConnectBody's vault branchreturn batwebsh.js:781sitsbefore the
if (rec.connection) b.connection = …line at:787, soconnectionis dropped from the vault POST body.Sending
rec.connectionand vetting viafind_config_connectionwould beunambiguous and rename-safe.
host:portmatching has two consequences:host:portwithdifferent
allowed_users/denied_users:find_prompt_connection_by_hostreturns the first, so the effective policy becomes file-order-dependent.
saved card silently breaks with the same "not allowed" this PR set out to
fix.
host:portwould still be needed as a fallback for legacy cards saved beforeconnection-name tagging (the
websh.js:2809auto-match path) — so:name-keyed primary,
host:portas a documented legacy shim.2. Scan-pattern detection skips vault rejections under
restrict_hostsUnder
restrict_hosts,_record_deny_for_scanis never called(
if not cfg["restrict_hosts"]). That was fine when a manual POST there wasjust a stale UI — but a saved-card POST is now a real accept/reject surface,
so host/slot probing under
restrict_hostsaccumulatesdeny_blockedandnever
scan_pattern, evading a fail2ban rule keyed on that signal. Feeding thescanner for
is_savedrejections would close it.3. No end-to-end test of the handler wiring
All 7 new tests call
authorize_target()directly. The handler path the PRactually ships —
is_savedderivation, the call site,host/portresolved-from-vault — has no coverage. An integration test through
/api/connectwould pin it. Also untested:ready-connection-not-matched(the function's core claim), port mismatch, and restrict-off + deny-listed
saved card.
4. Smaller items
c.get("host","") == host._parse_denied_hostsalready lowercases; a card whose host casing differsfrom
websh.jsonis falsely rejected. Casefolding both sides would match theexisting convention.
restrict_hostsdocs now predate the change — the "raw POSTs rejected /only via a named connection" wording no longer holds for the vault shape.
assertIn("not allowed", err)— that substring matches twodistinct reject reasons (and not the third), so a wrong-reason rejection
passes; exact
assertEqualwould be tighter.Happy to send a PR for the name-keyed approach + the cheap fixes if useful.