Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@ enum Commands {
Revert(command::revert::RevertArgs),
#[command(subcommand, about = "Manage set of tracked repositories")]
Remote(command::remote::RemoteCmds),
#[command(about = "Open the repository in the browser")]
Open(command::open::OpenArgs),
#[command(about = "Manage repository configurations")]
Config(command::config::ConfigArgs),
#[command(about = "Manage the log of reference changes (e.g., HEAD, branches)")]
Expand Down Expand Up @@ -209,6 +211,7 @@ pub async fn parse_async(args: Option<&[&str]>) -> Result<(), GitError> {
Commands::Blame(args) => command::blame::execute(args).await,
Commands::Revert(args) => command::revert::execute(args).await,
Commands::Remote(cmd) => command::remote::execute(cmd).await,
Commands::Open(args) => command::open::open(args).await,
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function call should be 'command::open::execute(args)' instead of 'command::open::open(args)' to match the naming convention established in this file. All other commands use the execute function (see lines 209-213: blame::execute, revert::execute, remote::execute, pull::execute, config::execute, etc.).

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Open command should be allowed to run outside a repository when a URL is provided directly as an argument (as the code supports on lines 33-36 of open.rs). However, the CLI framework at src/cli.rs:174-180 requires all commands except Init and Clone to be run within a repository, returning GitError::RepoNotFound otherwise. This prevents the use case of "libra open https://github.com/user/repo" from outside a repository. Consider adding Commands::Open to the exception list alongside Init and Clone, or redesign the command to always require being in a repository.

Copilot uses AI. Check for mistakes.
Commands::Pull(args) => command::pull::execute(args).await,
Commands::Config(args) => command::config::execute(args).await,
Commands::Checkout(args) => command::checkout::execute(args).await,
Expand Down
1 change: 1 addition & 0 deletions src/command/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ pub mod init;
pub mod lfs;
pub mod log;
pub mod merge;
pub mod open;
pub mod pull;
pub mod push;
pub mod rebase;
Expand Down
114 changes: 114 additions & 0 deletions src/command/open.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
use std::process::Command;
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing module-level documentation. All other command modules in this codebase include a //! doc comment at the top describing the module's purpose (see add.rs, blame.rs, branch.rs, etc.). Add a module-level doc comment following the established convention, e.g., "//! Opens the remote repository URL in the default browser by transforming Git URLs to HTTPS format."

Copilot uses AI. Check for mistakes.

use clap::Parser;
use regex::Regex;

use crate::internal::config::Config;

#[derive(Parser, Debug)]
pub struct OpenArgs {
/// The remote to open
pub remote: Option<String>,
}

pub async fn open(args: OpenArgs) {
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function name 'open' is inconsistent with the established naming convention in this codebase. All other command modules use 'execute' as the entry point function name (see add.rs:58, blame.rs:45, branch.rs:56, etc.). For consistency and adherence to codebase conventions, this function should be renamed to 'execute' and should take 'OpenArgs' as its parameter, matching the pattern 'pub async fn execute(args: OpenArgs)'.

Copilot uses AI. Check for mistakes.
let remote_url = if let Some(remote_name) = args.remote {
let remotes = Config::all_remote_configs().await;
if remotes.iter().any(|r| r.name == remote_name) {
Config::get_remote_url(&remote_name).await
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a remote name is specified, Config::get_remote_url can panic if the remote exists but has no URL configured (as seen in src/internal/config.rs:147). This is inconsistent with the check on line 17 which verifies the remote exists. Consider handling this potential panic case gracefully with an error message, or ensure the remote_config check on line 17 also validates that a URL is present.

Suggested change
if remotes.iter().any(|r| r.name == remote_name) {
Config::get_remote_url(&remote_name).await
if let Some(remote) = remotes.iter().find(|r| r.name == remote_name) {
if remote.url.is_empty() {
eprintln!("fatal: Remote '{}' has no URL configured", remote_name);
return;
}
remote.url.clone()

Copilot uses AI. Check for mistakes.
} else {
eprintln!("fatal: Remote '{}' not found", remote_name);
return;
}
} else {
match Config::get_current_remote_url().await {
Some(url) => url,
Comment on lines +42 to +43

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid panic on detached HEAD in open default path

libra open calls Config::get_current_remote_url() here when no remote is supplied; that helper unwrap()s and panics if HEAD is detached. In that scenario, the command crashes before reaching the fallback to origin/first remote, so running open in a detached HEAD state will unexpectedly abort. Consider handling the detached-HEAD error and continuing to the fallback branch instead of unwrapping.

Useful? React with 👍 / 👎.

None => {
Comment on lines +42 to +44

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Guard against detached HEAD before resolving default remote

When no remote is provided, this path calls Config::get_current_remote_url().await directly. That helper unwraps the result of get_current_remote_with_conn (see src/internal/config.rs around the unwrap() at lines 152-154), which panics on detached HEAD. Running libra open in a detached HEAD repo will therefore crash instead of emitting a user-friendly error. Consider checking get_current_remote_with_conn for Err (or providing a safe wrapper) before calling here so the command can report the detached HEAD condition gracefully.

Useful? React with 👍 / 👎.

// Fallback, try origin
let remotes = Config::all_remote_configs().await;
if let Some(origin) = remotes.iter().find(|r| r.name == "origin") {
origin.url.clone()
} else if let Some(first) = remotes.first() {
first.url.clone() // Fallback to first available remote
} else {
eprintln!("fatal: No remote configured");
return;
}
}
}
Comment on lines +42 to +56
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fallback logic for finding a remote has potential issues. When Config::get_current_remote_url() returns None, the code falls back to "origin", then to the first remote. However, this behavior is not clearly communicated to the user. Consider adding informational messages like "Using remote 'origin'" or "Using remote 'X' (first available)" so users understand which remote is being opened. This is especially important when multiple remotes exist.

Copilot uses AI. Check for mistakes.
};

let url = transform_url(&remote_url);
println!("Opening {}", url);

#[cfg(target_os = "windows")]
{
Command::new("cmd").args(["/C", "start", &url]).spawn().ok();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security concern: Command injection vulnerability

The URL is passed directly to the system shell without proper escaping. A malicious remote URL could potentially execute arbitrary commands.

Example attack vector:

remote.origin.url = "https://example.com; rm -rf /"

Recommendation:
Either:

  1. Validate and sanitize the URL before passing it to the shell command
  2. Use proper argument escaping/quoting
  3. Validate URL format with regex before execution

Example fix:

// Validate URL format
if !url.starts_with("http://") && !url.starts_with("https://") {
    eprintln!("fatal: Invalid URL format");
    return;
}

// On Windows, use proper argument escaping
Command::new("cmd")
    .args(["/C", "start", ""])  // Empty string forces URL to be treated as argument
    .arg(&url)
    .spawn()
    .ok();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Tyooughtul , 对于注入的安全问题,请参考 Claude 的 Review 意见进行修复

}
#[cfg(target_os = "macos")]
{
Command::new("open").arg(&url).spawn().ok();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security concern: Command injection vulnerability (macOS)

Same issue as Windows - the URL needs proper escaping. On macOS, open interprets certain characters specially.

Recommendation:

Command::new("open")
    .arg("--")  // Ensures URL is treated as argument, not option
    .arg(&url)
    .spawn()
    .ok();

}
#[cfg(all(not(target_os = "windows"), not(target_os = "macos")))]
{
Command::new("xdg-open").arg(&url).spawn().ok();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security concern: Command injection vulnerability (Linux)

Same issue - xdg-open can be exploited with malicious URLs.

Recommendation:

Command::new("xdg-open")
    .arg(&url)
    .spawn()
    .ok();

Note: Using .arg() separately (not in the command string) provides better protection than shell interpolation.

Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Command::spawn() calls have their errors silently discarded with .ok(). If the browser fails to open (e.g., command not found, permission denied), the user will see "Opening URL" but the browser won't actually open and they'll get no indication of what went wrong. Consider handling the error and providing user feedback. For example, you could check the Result and print a warning if the spawn fails.

Suggested change
Command::new("cmd").args(["/C", "start", &url]).spawn().ok();
}
#[cfg(target_os = "macos")]
{
Command::new("open").arg(&url).spawn().ok();
}
#[cfg(all(not(target_os = "windows"), not(target_os = "macos")))]
{
Command::new("xdg-open").arg(&url).spawn().ok();
if let Err(err) = Command::new("cmd").args(["/C", "start", &url]).spawn() {
eprintln!("warning: failed to open URL '{}' in browser: {}", url, err);
}
}
#[cfg(target_os = "macos")]
{
if let Err(err) = Command::new("open").arg(&url).spawn() {
eprintln!("warning: failed to open URL '{}' in browser: {}", url, err);
}
}
#[cfg(all(not(target_os = "windows"), not(target_os = "macos")))]
{
if let Err(err) = Command::new("xdg-open").arg(&url).spawn() {
eprintln!("warning: failed to open URL '{}' in browser: {}", url, err);
}

Copilot uses AI. Check for mistakes.
}
}
Comment on lines 78 to 99
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error handling: Silent failures

All three platform-specific commands use .ok() which silently ignores errors. Users won't know if:

  • The command doesn't exist on their system
  • The URL format is invalid
  • The browser launch failed

Recommendation:

let result = Command::new("open").arg(&url).spawn();
match result {
    Ok(_) => {},
    Err(e) => eprintln!("warning: Failed to open browser: {}", e),
}


fn transform_url(remote: &str) -> String {
if remote.starts_with("http://") || remote.starts_with("https://") {
return remote.trim_end_matches(".git").to_string();
}

// Handle SCP-like syntax: git@github.com:user/repo.git
let scp_re = Regex::new(r"^git@([^:]+):(.+?)(\.git)?$").unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential panic: unwrap() on regex

While this regex is valid and won't panic in practice, using unwrap() on Regex::new() is considered a code smell.

Best practice:
Use lazy_static or once_cell to compile the regex once at startup:

use once_cell::sync::Lazy;

static SCP_RE: Lazy<Regex> = Lazy::new(|| {
    Regex::new(r"^git@([^:]+):(.+?)(\.git)?$").expect("Invalid SCP regex")
});

// Then use: if let Some(caps) = SCP_RE.captures(remote) { ... }

This is more efficient and makes the panic cause clearer if it happens during initialization.

if let Some(caps) = scp_re.captures(remote) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Parse SCP-style SSH URLs with non-git user

The SCP-like parser only matches git@host:path (^git@...) so remotes like alice@github.com:org/repo.git (or enterprise setups with a different SSH user) won’t be transformed into an HTTPS URL. Those inputs fall through to xdg-open/open as a raw SSH string, which typically can’t be opened in a browser, so libra open silently fails for a common remote format. The regex should accept any user@ prefix (or parse generically) to cover these cases.

Useful? React with 👍 / 👎.

let host = &caps[1];
let path = &caps[2];
return format!("https://{}/{}", host, path);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code clarity: Direct string formatting

Minor: The format! macro can be simplified when concatenating with /:

return format!("https://{host}/{path}");

This is more idiomatic Rust.

}
Comment on lines 106 to 111
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SCP-style regex pattern (line 64) assumes "git@" as the user, but SCP syntax can use any username. For example, "john@gitlab.com:repo/project.git" is valid Git syntax but won't match this pattern. Consider making the user part generic by changing the regex to match any user, e.g., r"^([^@]+)@([^:]+):(.+?)(\.git)?$" and capturing both user and host, or at minimum updating the regex to handle any username.

Copilot uses AI. Check for mistakes.

// Handle ssh:// syntax
// ssh://[user@]host.xz[:port]/path/to/repo.git/
if remote.starts_with("ssh://") {
let ssh_re = Regex::new(r"^ssh://(?:[^@]+@)?([^:/]+)(?::\d+)?/(.+?)(\.git)?$").unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue: unwrap() on regex

Same recommendation as above - use lazy_static or once_cell for the SSH regex.

Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Regex compilation happens on every call to transform_url. According to Rust performance best practices and the custom guidelines emphasizing performance optimization, these regexes should be compiled once using lazy_static or std::sync::OnceLock to avoid repeated compilation overhead. This is especially important as this function is in a hot path called for every URL transformation.

Copilot uses AI. Check for mistakes.
if let Some(caps) = ssh_re.captures(remote) {
let host = &caps[1];
let path = &caps[2];
return format!("https://{}/{}", host, path);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above: Can simplify to format!("https://{host}/{path}")

}
}

// Fallback: return as is, maybe it is already workable or user has weird config
remote.to_string()
}

// Uint test
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in the comment. Should be "Unit test" instead of "Uint test".

Suggested change
// Uint test
// Unit test

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in comment: "Uint test" should be "Unit test".

Suggested change
// Uint test
// Unit test

Copilot uses AI. Check for mistakes.
#[cfg(test)]
mod tests {
use super::*;

#[test]
fn test_transform_url() {
assert_eq!(
transform_url("git@github.com:rust-lang/rust.git"),
"https://github.com/rust-lang/rust"
);
assert_eq!(
transform_url("git@gitlab.com:group/project.git"),
"https://gitlab.com/group/project"
);
assert_eq!(
transform_url("https://github.com/rust-lang/rust.git"),
"https://github.com/rust-lang/rust"
);
assert_eq!(
transform_url("ssh://git@github.com/rust-lang/rust.git"),
"https://github.com/rust-lang/rust"
);
assert_eq!(
transform_url("ssh://user@host.com:2222/repo.git"),
"https://host.com/repo"
);
}
Comment on lines +131 to +152
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unit tests don't cover several important edge cases:

  1. URLs with usernames other than 'git' in SCP format (e.g., 'myuser@github.com:repo/path.git')
  2. HTTPS URLs with trailing slashes
  3. URLs without .git suffix
  4. Malformed URLs that should fail gracefully
  5. File:// URLs (should be rejected by is_safe_url)
  6. URLs with query parameters or fragments

Consider adding test cases for these scenarios to ensure robust URL handling.

Copilot uses AI. Check for mistakes.
}
1 change: 1 addition & 0 deletions tests/command/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ mod init_test;
mod lfs_test;
mod log_test;
mod merge_test;
mod open_test;
mod pull_test;
mod push_test;
mod rebase_test;
Expand Down
34 changes: 34 additions & 0 deletions tests/command/open_test.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
//! Tests open command integration to ensure it finds remote correctly.
use libra::{
command::{
open,
remote::{self, RemoteCmds},
},
utils::test,
};
use serial_test::serial;
use tempfile::tempdir;

#[tokio::test]
#[serial]
async fn test_open_remote_origin() {
let repo_dir = tempdir().unwrap();
test::setup_with_new_libra_in(repo_dir.path()).await;
let _guard = test::ChangeDirGuard::new(repo_dir.path());

// Add origin remote
remote::execute(RemoteCmds::Add {
name: "origin".into(),
url: "git@github.com:user/repo.git".into(),
})
.await;

// Test explicit remote
open::open(open::OpenArgs {
remote: Some("origin".to_string()),
})
.await;

// Test default remote should find origin
open::open(open::OpenArgs { remote: None }).await;
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The integration test doesn't verify the expected behavior - it calls the open function but doesn't assert anything. The test should verify that:

  1. The correct remote URL is identified
  2. The URL is properly transformed
  3. The operation completes without errors

Consider capturing stdout/stderr or mocking the browser launch to verify the correct URL is being opened.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test calls 'open::open' but according to the codebase convention established in this PR review, the function should be named 'execute' (see src/cli.rs and other command modules). Once the function is renamed, this test call should also be updated to 'open::execute'.

Copilot uses AI. Check for mistakes.
}
Comment on lines 12 to 51
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test coverage: Limited assertions

The test doesn't verify:

  1. That the correct URL was opened (no way to capture what was sent to the browser)
  2. That the URL transformation worked correctly
  3. Different remote URL formats (HTTPS, SSH, SCP)
  4. Error cases (no remote, invalid remote name)

Recommendation:
Consider adding unit tests for the transform_url function (which you already have ✓) and integration tests for error scenarios:

#[tokio::test]
#[serial]
async fn test_open_nonexistent_remote() {
    let repo_dir = tempdir().unwrap();
    test::setup_with_new_libra_in(repo_dir.path()).await;
    let _guard = test::ChangeDirGuard::new(repo_dir.path());
    
    // Should print error message and not panic
    open::open(open::OpenArgs {
        remote: Some("nonexistent".to_string()),
    })
    .await;
}

#[tokio::test]
#[serial]
async fn test_open_no_remotes_configured() {
    let repo_dir = tempdir().unwrap();
    test::setup_with_new_libra_in(repo_dir.path()).await;
    let _guard = test::ChangeDirGuard::new(repo_dir.path());
    
    // Should print error message and not panic
    open::open(open::OpenArgs { remote: None }).await;
}

Comment on lines 44 to 51
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test lacks assertions. It calls open::open() with no remote configured, but doesn't verify:

  1. That an appropriate error message is printed
  2. That the function handles the error gracefully

Without assertions, this test will pass even if the function panics or behaves incorrectly. Add assertions to verify expected error output or return values.

Copilot uses AI. Check for mistakes.