Conversation
Five concrete fixes from Copilot's review of #73 (one false positive about a `shift 2>/dev/null` mis-read as `shift 2` left as-is): 1. **kick was removing the wrong pubkey** — `<peer>.pub` holds the signing key (sign_pub), not the SSH auth key. cmd_kick was reading that file and trying to grep it out of authorized_keys, which never matched. Result: kicked peers kept SSH access. Now host handshake stores `ssh_pub` in the peer JSON record; cmd_kick reads from there. Falls back to a clear "no SSH key recorded" warning for pre-#34 peers (whose handshake didn't carry ssh_pub). 2. **path traversal in cmd_whois / cmd_kick** — peer name was interpolated into local file paths AND remote SSH cat commands without sanitization. `airc whois ../config` would have been happily resolved against the joiner's host. New _validate_peer_name helper enforces `[a-z0-9-]+` (same charset cmd_rename already uses), no leading dash. Wired into both commands. 3. **kick test never asserted authorized_keys removal** — scenario covered the peer file removal but not the SSH key revocation, which is the actual security boundary. Now greps authorized_keys for the joiner's pubkey before/after kick. Would have caught (1) immediately. 4. **identity link usage string lied** — claimed `<handle>` was required, but the code actually unlinks when handle is omitted. Updated usage + help to reflect optional/blank handle = unlink. 5. **path-traversal regression test** — scenario_kick now also asserts that `airc whois ../config` and `airc kick ../config` both fail with "invalid peer name". Defense in depth. 130/130 integration tests pass (4 new). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per Joel: airc is the chat substrate that came out of continuum work, and the broader claim is that collaborative agentic systems are the unlock. One italic line right under the title to set context for fresh-clone readers before they hit the install instructions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses security/correctness issues in peer management (kick/whois) and tightens integration coverage around SSH access revocation, plus a small README tagline update.
Changes:
- Persist joiner
ssh_pubin peer JSON records and use it inairc kickto actually revoke SSH access viaauthorized_keysremoval. - Add
_validate_peer_nameand apply it towhois/kickto prevent path traversal / injection via peer names. - Strengthen
scenario_kickto assertauthorized_keysremoval and reject traversal-style peer names; updateidentity linkhelp text; add README tagline.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
airc |
Stores ssh_pub in peer JSON, validates peer names for whois/kick, updates kick logic and identity-link usage text. |
test/integration.sh |
Adds pre/post assertions that kick removes the joiner’s SSH key from authorized_keys and rejects traversal peer names. |
README.md |
Adds a short italicized tagline under the title. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+180
to
+191
| # Validate a peer name matches the allowed nick charset BEFORE using it in | ||
| # filesystem paths or remote SSH commands. Same charset cmd_rename uses | ||
| # (a-z 0-9 -) — anything else is unreachable as a real airc identity and | ||
| # almost certainly a path-traversal / shell-injection attempt | ||
| # (`../config`, `; rm -rf /`, etc.). Rejects empty as well. | ||
| _validate_peer_name() { | ||
| local name="${1:-}" | ||
| case "$name" in | ||
| "") die "peer name required" ;; | ||
| *[!a-z0-9-]*) die "invalid peer name '$name' — must match [a-z0-9-]+ (use airc peers to list valid names)" ;; | ||
| -*) die "invalid peer name '$name' — must not start with '-'" ;; | ||
| esac |
There was a problem hiding this comment.
_validate_peer_name rejects names starting with '-' but cmd_rename can still produce a leading '-' after sanitization (e.g., renaming to '.foo' becomes '-foo'). That would make such a peer name impossible to whois/kick and contradicts the "reject leading dash" intent. Consider trimming leading '-' after cmd_rename sanitization and re-validating, or otherwise aligning name-generation rules with this validator.
Comment on lines
+1345
to
+1352
| # ── Capture joiner's SSH pubkey BEFORE kick so we can assert removal ── | ||
| local kj_ssh_pub | ||
| kj_ssh_pub=$(cat /tmp/airc-it-k-j/state/identity/ssh_key.pub 2>/dev/null | tr -d '\n' || true) | ||
| if [ -n "$kj_ssh_pub" ] && [ -f "$HOME/.ssh/authorized_keys" ]; then | ||
| grep -qF "$kj_ssh_pub" "$HOME/.ssh/authorized_keys" \ | ||
| && pass "joiner's SSH key present in authorized_keys before kick" \ | ||
| || fail "joiner's SSH key missing from authorized_keys before kick (handshake regression?)" | ||
| fi |
There was a problem hiding this comment.
The new authorized_keys assertions are skipped entirely if kj_ssh_pub is empty or authorized_keys is missing. In this scenario, both should exist (init_identity generates ssh_key.pub and creates/updates authorized_keys), so skipping can let a regression slip through silently. Consider failing the test when the joiner pubkey can't be read or authorized_keys isn't present, so the kick-revocation check is always exercised.
joelteply
added a commit
that referenced
this pull request
Apr 25, 2026
fix: PR #75 Copilot follow-up + 'pure shell' title hyphen
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses Copilot's review on PR #73 and adds a README tagline per Joel.
Copilot fixes
cmd_kickwas reading<peer>.pub(signing key) from authorized_keys, not the SSH auth key — kicked peers retained SSH accessssh_pubin peer JSON; cmd_kick reads from therecmd_whois/cmd_kickinterpolated peer name into local + remote paths without sanitization (path traversal, shell injection vector)_validate_peer_namehelper,[a-z0-9-]+onlyauthorized_keysactually changedairc identity linkusage said handle was required, but empty handle unlinksOne Copilot comment was a false positive: it read
shift 2>/dev/nullasshift 2(count 2), but2>/dev/nullis stderr redirection — the shift count is 1, which is correct. No change needed.README tagline
One italic line right under the title:
Anvil DM'd separately for input — happy to revise to whatever Anvil's framing if she has a tighter line.
Test plan
airc whois ../configandairc kick ../configboth rejected with "invalid peer name"airc identity link continuum Earlthenairc identity link continuum(no handle) properly unlinks