-
Notifications
You must be signed in to change notification settings - Fork 0
Cross-platform robustness and edge cases #5
Description
Epic 7: CLI — Cross-platform robustness and edge cases
Feedback from PR reviewers on the Axios PR (axios/axios#10646) and
from auditing the CLI internals exposed several issues that affect
real-world adoption.
Task 7.1: auths verify — handle shallow repos and short commit histories gracefully
What: auths verify HEAD~2..HEAD~1 fails in a 2-commit repo because
HEAD~2 doesn't exist. The CLI calls git rev-list which emits
fatal: bad revision 'HEAD~2'. While the error is caught, the message
is cryptic and doesn't guide the user to a fix.
Why: The GitHub Action auto-detects commit ranges from PR context
(pr.base.sha..pr.head.sha) so this doesn't hit in CI. But example
scripts and manual CLI usage hit this constantly — especially in demo
repos or new projects with few commits.
File to modify:
/Users/bordumb/workspace/repositories/auths-base/auths/crates/auths-cli/src/commands/verify_commit.rs
Current behavior (lines 198-226):
fn resolve_commits(commit_spec: &str) -> Result<Vec<String>> {
if commit_spec.contains("..") {
let output = Command::new("git")
.args(["rev-list", commit_spec])
.output()?;
// On error: "Invalid commit range: {stderr}"
}
}Improve: Detect the "bad revision" error pattern and emit a
helpful message:
if stderr.contains("bad revision") || stderr.contains("unknown revision") {
return Err(anyhow!(
"Commit range '{}' references commits that don't exist. \
In a repo with N commits, HEAD~N is invalid. \
Try 'auths verify HEAD' for the latest commit, \
or use explicit SHAs.",
commit_spec
));
}Task 7.2: CLI — cross-platform ssh-keygen availability check
What: The CLI depends on ssh-keygen for commit signature verification
(ssh-keygen -Y find-principals and ssh-keygen -Y verify). On Windows
without OpenSSH installed, auths verify fails with a confusing error.
Why: The CLI already has a check_ssh_keygen() function (line 143 in
verify_commit.rs) that runs ssh-keygen -? as early validation. But the
error message could be more helpful with platform-specific install guidance.
File to modify:
/Users/bordumb/workspace/repositories/auths-base/auths/crates/auths-cli/src/commands/verify_commit.rs
Current check (line 143):
fn check_ssh_keygen() -> Result<()> {
Command::new("ssh-keygen").arg("-?").output()
.map_err(|_| anyhow!("ssh-keygen not found"))?;
Ok(())
}Improve: Add platform-specific guidance and minimum version check
(SSH signing requires OpenSSH 8.0+):
fn check_ssh_keygen() -> Result<()> {
let output = Command::new("ssh-keygen").arg("-V").output()
.map_err(|_| {
let install_hint = if cfg!(target_os = "macos") {
"ssh-keygen should be pre-installed on macOS."
} else if cfg!(target_os = "windows") {
"Install OpenSSH via Settings > Apps > Optional features, \
or install Git for Windows which bundles it."
} else {
"Install openssh-client: apt install openssh-client"
};
anyhow!("ssh-keygen not found on PATH.\n {}", install_hint)
})?;
// Check version >= 8.0 (required for -Y sign/verify)
// ...
Ok(())
}File reference (existing Windows hint):
/Users/bordumb/workspace/repositories/auths-base/auths/crates/auths-cli/src/errors/renderer.rs
already has a Windows install hint — consolidate with the check function.
Task 7.3: CLI — add auths doctor check for ssh-keygen version
What: The auths doctor command should verify that ssh-keygen is
available AND that it supports the -Y flag (SSH signing, added in
OpenSSH 8.0). Currently system_diagnostic.rs checks availability but
not version.
File to modify:
/Users/bordumb/workspace/repositories/auths-base/auths/crates/auths-cli/src/adapters/system_diagnostic.rs
Add a version-awareness check that parses ssh-keygen -V output
and warns if the version is < 8.0.
Epic 8: GitHub Action — Least-privilege permissions
PR review feedback (axios/axios#10646) flagged that the GitHub Action
workflow grants pull-requests: write on all triggers, including push,
where it's not needed.
Task 8.1: Condition pull-requests: write on event type
What: The auths-verify-commits.yml workflow grants
pull-requests: write at job level for all triggers. On push events,
this is unnecessary and expands the token's privilege surface.
Why: A reviewer correctly pointed out this is the same class of
over-permissioning that contributed to the original attacks. The Auths
action only uses pull-requests: write when post-pr-comment: true
AND the trigger is a pull_request event.
Recommendation for the GitHub Action itself:
File to modify:
/Users/bordumb/workspace/repositories/auths-base/auths-verify-github-action/src/main.ts
The action should skip the PR comment step (and not require the token)
when the event is not a pull_request. Currently the action may already
do this, but the workflow template should make it explicit by splitting
into two jobs or using conditional permissions:
Recommended workflow pattern for consumers:
jobs:
verify:
runs-on: ubuntu-latest
permissions:
contents: read
pull-requests: ${{ github.event_name == 'pull_request' && 'write' || 'none' }}File to also update (action README / examples):
/Users/bordumb/workspace/repositories/auths-base/auths-verify-github-action/README.md
Update all example workflows to use conditional permissions.
Task 8.2: Action should warn when post-pr-comment is true on non-PR events
What: If a user configures post-pr-comment: true but the action
runs on a push event, the action should log a warning rather than
silently skipping or failing.
File to modify:
/Users/bordumb/workspace/repositories/auths-base/auths-verify-github-action/src/main.ts
if (postPrComment && github.context.eventName !== 'pull_request') {
core.warning(
'post-pr-comment is enabled but this is not a pull_request event. ' +
'PR comment will be skipped. Consider conditioning pull-requests: write ' +
'permission on the event type.'
)
}