Repository navigation
Conversation
Documentation for the known_hosts file format specifies "Hostnames is a comma-separated list of patterns [...]". The regexps we had is unnecessarily strict, allowing only a subset of valid sequences. Make it more lenient (and simple) to match more valid ones.
a136272 to
7fd4b00
Compare
|
Thanks for this. I would love to get some tests in place as changing regexps is otherwise very painful. Any chance you could be persuaded into adding a test or two for this as part of the PR? |
📝 WalkthroughWalkthroughThe change adds a reusable regex for hostname, IP address, and bracketed host-port forms. The known_hosts font-lock regex uses this pattern to match comma-separated host lists. ChangesKnown hosts regex handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ssh-config-mode.el`:
- Around line 334-338: Add a regression font-lock test covering a known-host
entry with multiple comma-separated IP addresses, such as
host.example.com,192.0.2.1,192.0.2.2. Verify that the complete hostname list
receives the expected host face and that each subsequent key field retains its
expected face.
- Around line 304-313: Add ssh-known-hosts-regex-hostname-pattern to the cleanup
list used by ssh-known-hosts-mode-reload in the test helper, so reloading tests
clears the defvar-bound value and rebuilds the pattern.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| (defvar ssh-known-hosts-regex-hostname-pattern | ||
| (concat | ||
| "\\(?:" | ||
| ssh-known-hosts-regex-hostname | ||
| "\\|" | ||
| ssh-known-hosts-regex-ip | ||
| "\\|" | ||
| ;; [host-or-ip]:222 | ||
| "\\(?:\\[" ssh-known-hosts-regex-host "\\]:" ssh-known-hosts-regex-port "\\)" | ||
| "\\)")) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 \
'ssh-known-hosts-regex-hostname-pattern|makunbound|ssh-known-hosts-mode-reload' \
ssh-config-mode.el tests/ssh-config-test-helper-funcs.elRepository: peterhoeg/ssh-config-mode-el
Length of output: 1896
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- reload helper ---'
cat -n tests/ssh-config-test-helper-funcs.el | sed -n '1,55p'
printf '%s\n' '--- regex definitions and uses ---'
cat -n ssh-config-mode.el | sed -n '270,350p'
printf '%s\n' '--- test references ---'
rg -n -C 3 \
'ssh-known-hosts-mode-reload|hostname-pattern|comma|known-hosts' \
tests --glob '*.el' | sed -n '1,240p'Repository: peterhoeg/ssh-config-mode-el
Length of output: 5848
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if command -v emacs >/dev/null 2>&1; then
emacs --batch --quick --eval '
(progn
(makunbound (quote probe-base))
(makunbound (quote probe-derived))
(eval (quote (progn
(defvar probe-base "old")
(defvar probe-derived (concat probe-base "-derived")))))
(let ((first probe-derived))
(makunbound (quote probe-base))
(eval (quote (progn
(defvar probe-base "new")
(defvar probe-derived (concat probe-base "-derived")))))
(princ (format "first=%S second=%S same=%s\n"
first probe-derived (eq first probe-derived))))))
'
else
printf '%s\n' 'Emacs is unavailable'
fiRepository: peterhoeg/ssh-config-mode-el
Length of output: 187
Reset ssh-known-hosts-regex-hostname-pattern during test reload.
When ssh-known-hosts-mode-reload runs again, defvar preserves the existing pattern. Add ssh-known-hosts-regex-hostname-pattern to the cleanup list in tests/ssh-config-test-helper-funcs.el.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ssh-config-mode.el` around lines 304 - 313, Add
ssh-known-hosts-regex-hostname-pattern to the cleanup list used by
ssh-known-hosts-mode-reload in the test helper, so reloading tests clears the
defvar-bound value and rebuilds the pattern.
| ;; list of comma separated hostname patterns | ||
| ssh-known-hosts-regex-hostname-pattern | ||
| "\\(?:," | ||
| ssh-known-hosts-regex-hostname-pattern | ||
| "\\)*" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add a regression test for hostname lists with multiple IP addresses.
Add a font-lock test for an entry such as host.example.com,192.0.2.1,192.0.2.2. Verify that the host field and subsequent key fields receive their expected faces.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ssh-config-mode.el` around lines 334 - 338, Add a regression font-lock test
covering a known-host entry with multiple comma-separated IP addresses, such as
host.example.com,192.0.2.1,192.0.2.2. Verify that the complete hostname list
receives the expected host face and that each subsequent key field retains its
expected face.
Documentation for the known_hosts file format specifies "Hostnames is a comma-separated list of patterns [...]". The regexps we had is unnecessariliy strict, allowing only a subset of valid sequences.
Make it more lenient (and simple) to match more valid ones.
https://man.openbsd.org/sshd#SSH_KNOWN_HOSTS_FILE_FORMAT
Wildcard support is still missing, but I'm scratching another itch I have with this (manually crafted entries with hostname followed by more than one IP address).
Summary by CodeRabbit
known_hostsfiles.