refactor(server): replace custom remote clients with macro-based implementation

- Replaced manual remote client functions with remote_client! macro for archive, blame, branch, commit, and diff services
- Simplified remote client creation logic using declarative macro approach
- Maintained same functionality while reducing code duplication across services

security(bare): enhance path traversal protection with comprehensive validation

- Added early relative_path validation to prevent path traversal attacks
- Implemented unified path validation to avoid TOCTOU race conditions
- Enhanced canonicalization checks for both existing and non-existent paths
- Added detailed logging for path traversal detection attempts

feat(cache): migrate from CLruCache to Moka with TTL and invalidation support

- Replaced clru dependency with moka for improved caching capabilities
- Added 300-second time-to-live for cache entries
- Implemented repository-specific cache invalidation mechanism
- Enhanced cache operations with thread-safe async support

refactor(commit): improve security validation for commit operations

- Added ref name validation to prevent command injection in cherry_pick_commit
- Implemented revision validation for commit selectors
- Added comprehensive input validation for create_commit parameters
- Enhanced file path validation to prevent traversal

tests/archive_test.rs

@@ -13,7 +13,7 @@ fn hdr(name: &str) -> RepositoryHeader {
 
 #[tokio::test]
 async fn test_get_archive_tar() {
-    let (dir, gb) = common::setup_bare_repo();
+    let (dir, _gb) = common::setup_bare_repo();
     let svc = common::setup_service(dir.path());
     let chunks = svc
         .get_archive(tonic::Request::new(ArchiveRequest {
@@ -40,7 +40,7 @@ async fn test_get_archive_tar() {
 
 #[tokio::test]
 async fn test_get_archive_zip() {
-    let (dir, gb) = common::setup_bare_repo();
+    let (dir, _gb) = common::setup_bare_repo();
     let svc = common::setup_service(dir.path());
     let chunks = svc
         .get_archive(tonic::Request::new(ArchiveRequest {
@@ -70,7 +70,7 @@ async fn test_get_archive_zip() {
 
 #[tokio::test]
 async fn test_list_archive_entries() {
-    let (dir, gb) = common::setup_bare_repo();
+    let (dir, _gb) = common::setup_bare_repo();
     let svc = common::setup_service(dir.path());
     let result = svc
         .list_archive_entries(tonic::Request::new(ListArchiveEntriesRequest {
@@ -98,7 +98,7 @@ async fn test_list_archive_entries() {
 
 #[tokio::test]
 async fn test_get_archive_with_prefix() {
-    let (dir, gb) = common::setup_bare_repo();
+    let (dir, _gb) = common::setup_bare_repo();
     let svc = common::setup_service(dir.path());
     let chunks = svc
         .get_archive(tonic::Request::new(ArchiveRequest {
@@ -124,7 +124,7 @@ async fn test_get_archive_with_prefix() {
 
 #[tokio::test]
 async fn test_fsck_clean_repo() {
-    let (dir, gb) = common::setup_bare_repo();
+    let (dir, _gb) = common::setup_bare_repo();
     let svc = common::setup_service(dir.path());
     let result = svc
         .fsck(tonic::Request::new(FsckRequest {
@@ -209,7 +209,7 @@ async fn test_list_packfiles() {
 
 #[tokio::test]
 async fn test_advertise_refs() {
-    let (dir, gb) = common::setup_bare_repo();
+    let (dir, _gb) = common::setup_bare_repo();
     let svc = common::setup_service(dir.path());
     let result = svc
         .advertise_refs(tonic::Request::new(AdvertiseRefsRequest {

tests/bare_test.rs

@@ -6,7 +6,7 @@ use gitks::pb::RepositoryHeader;
 
 #[test]
 fn test_from_header_valid() {
-    let (dir, gb) = common::setup_bare_repo();
+    let (_dir, gb) = common::setup_bare_repo();
     let parent = gb.bare_dir.parent().unwrap().to_string_lossy().into_owned();
     let name = gb
         .bare_dir

tests/blame_test.rs

@@ -124,11 +124,11 @@ async fn test_blame_author_info() {
 
     let hunk = &result.hunks[0];
     let commit = hunk.commit.as_ref().unwrap();
-    if let Some(ref author) = commit.author {
-        if let Some(ref id) = author.identity {
-            assert_eq!(id.name, "Test", "author name should match");
-            assert_eq!(id.email, "test@example.com");
-        }
+    if let Some(ref author) = commit.author
+        && let Some(ref id) = author.identity
+    {
+        assert_eq!(id.name, "Test", "author name should match");
+        assert_eq!(id.email, "test@example.com");
     }
 }
 

tests/common/mod.rs

@@ -1,6 +1,7 @@
 use gitks::bare::GitBare;
 use gitks::server::GitksService;
 
+#[allow(dead_code)]
 pub fn setup_service(dir: &std::path::Path) -> GitksService {
     GitksService::new(dir.to_path_buf())
 }
@@ -104,6 +105,7 @@ pub fn setup_bare_repo() -> (tempfile::TempDir, GitBare) {
     (dir, GitBare::new(bare_dir))
 }
 
+#[allow(dead_code)]
 pub fn setup_bare_repo_with_conflict() -> (tempfile::TempDir, GitBare) {
     let dir = tempfile::tempdir().expect("create temp dir");
     let bare_dir = dir.path().join("test-repo");

tests/macro_test.rs

@@ -0,0 +1,184 @@
+use gitks::error::GitResult;
+use gitks::pb::object_selector::Selector;
+use gitks::pb::{ObjectName, ObjectSelector};
+
+fn test_macro(selector: Option<ObjectSelector>) -> GitResult<String> {
+    let result = gitks::resolve_revision!(selector);
+    Ok(result)
+}
+
+fn test_macro_with_default(selector: Option<ObjectSelector>, default: &str) -> GitResult<String> {
+    let result = gitks::resolve_revision!(selector, default);
+    Ok(result)
+}
+
+#[test]
+fn test_resolve_revision_with_oid() {
+    let selector = Some(ObjectSelector {
+        selector: Some(Selector::Oid(gitks::pb::Oid {
+            hex: "abc1234567890123456789012345678901234567".to_string(),
+            value: vec![],
+            format: gitks::pb::ObjectFormat::Sha1 as i32,
+        })),
+    });
+
+    let result = test_macro(selector).unwrap();
+    assert_eq!(result, "abc1234567890123456789012345678901234567");
+}
+
+#[test]
+fn test_resolve_revision_with_valid_revision() {
+    let selector = Some(ObjectSelector {
+        selector: Some(Selector::Revision(ObjectName {
+            revision: "main".to_string(),
+        })),
+    });
+
+    let result = test_macro(selector).unwrap();
+    assert_eq!(result, "main");
+}
+
+#[test]
+fn test_resolve_revision_with_valid_branch() {
+    let selector = Some(ObjectSelector {
+        selector: Some(Selector::Revision(ObjectName {
+            revision: "feature/new-api".to_string(),
+        })),
+    });
+
+    let result = test_macro(selector).unwrap();
+    assert_eq!(result, "feature/new-api");
+}
+
+#[test]
+fn test_resolve_revision_with_head() {
+    let selector = Some(ObjectSelector {
+        selector: Some(Selector::Revision(ObjectName {
+            revision: "HEAD".to_string(),
+        })),
+    });
+
+    let result = test_macro(selector).unwrap();
+    assert_eq!(result, "HEAD");
+}
+
+#[test]
+fn test_resolve_revision_with_ancestry() {
+    let selector = Some(ObjectSelector {
+        selector: Some(Selector::Revision(ObjectName {
+            revision: "main~3".to_string(),
+        })),
+    });
+
+    let result = test_macro(selector).unwrap();
+    assert_eq!(result, "main~3");
+}
+
+#[test]
+fn test_resolve_revision_none_defaults_to_head() {
+    let result = test_macro(None).unwrap();
+    assert_eq!(result, "HEAD");
+}
+
+#[test]
+fn test_resolve_revision_with_custom_default() {
+    let result = test_macro_with_default(None, "develop").unwrap();
+    assert_eq!(result, "develop");
+}
+
+#[test]
+fn test_resolve_revision_empty_selector() {
+    let selector = Some(ObjectSelector { selector: None });
+    let result = test_macro(selector).unwrap();
+    assert_eq!(result, "HEAD");
+}
+
+#[test]
+fn test_resolve_revision_empty_with_custom_default() {
+    let selector = Some(ObjectSelector { selector: None });
+    let result = test_macro_with_default(selector, "custom-branch").unwrap();
+    assert_eq!(result, "custom-branch");
+}
+
+#[test]
+fn test_resolve_revision_rejects_dangerous() {
+    let selector = Some(ObjectSelector {
+        selector: Some(Selector::Revision(ObjectName {
+            revision: "branch;rm -rf /".to_string(),
+        })),
+    });
+
+    let result = test_macro(selector);
+    assert!(result.is_err());
+}
+
+#[test]
+fn test_resolve_revision_rejects_traversal() {
+    let selector = Some(ObjectSelector {
+        selector: Some(Selector::Revision(ObjectName {
+            revision: "../etc/passwd".to_string(),
+        })),
+    });
+
+    let result = test_macro(selector);
+    assert!(result.is_err());
+}
+
+#[test]
+fn test_resolve_revision_rejects_excessive_depth() {
+    let selector = Some(ObjectSelector {
+        selector: Some(Selector::Revision(ObjectName {
+            revision: "main~99999".to_string(),
+        })),
+    });
+
+    let result = test_macro(selector);
+    assert!(result.is_err());
+}
+
+#[test]
+fn test_resolve_revision_rejects_too_long() {
+    let long_rev = "a".repeat(300);
+    let selector = Some(ObjectSelector {
+        selector: Some(Selector::Revision(ObjectName { revision: long_rev })),
+    });
+
+    let result = test_macro(selector);
+    assert!(result.is_err());
+}
+
+#[test]
+fn test_resolve_revision_accepts_valid_hex() {
+    let selector = Some(ObjectSelector {
+        selector: Some(Selector::Revision(ObjectName {
+            revision: "deadbeef1234567890abcdef".to_string(),
+        })),
+    });
+
+    let result = test_macro(selector).unwrap();
+    assert_eq!(result, "deadbeef1234567890abcdef");
+}
+
+#[test]
+fn test_resolve_revision_accepts_ref_prefix() {
+    let selector = Some(ObjectSelector {
+        selector: Some(Selector::Revision(ObjectName {
+            revision: "ref:refs/heads/main".to_string(),
+        })),
+    });
+
+    let result = test_macro(selector).unwrap();
+    assert_eq!(result, "ref:refs/heads/main");
+}
+
+#[test]
+fn test_resolve_revision_accepts_tree_suffix() {
+    let selector = Some(ObjectSelector {
+        selector: Some(Selector::Revision(ObjectName {
+            revision: "main^{tree}".to_string(),
+        })),
+    });
+
+    let result = test_macro(selector).unwrap();
+    assert_eq!(result, "main^{tree}");
+}

tests/sanitize_test.rs

@@ -0,0 +1,283 @@
+use gitks::sanitize::*;
+
+// ==================== validate_ref_name tests ====================
+
+#[test]
+fn test_validate_ref_name_accepts_valid_names() {
+    assert!(validate_ref_name("main").is_ok());
+    assert!(validate_ref_name("master").is_ok());
+    assert!(validate_ref_name("feature/new-api").is_ok());
+    assert!(validate_ref_name("hotfix/bug-fix-123").is_ok());
+    assert!(validate_ref_name("v1.0.0").is_ok());
+    assert!(validate_ref_name("release-2024").is_ok());
+    assert!(validate_ref_name("user/feature/branch").is_ok());
+}
+
+#[test]
+fn test_validate_ref_name_rejects_empty() {
+    assert!(validate_ref_name("").is_err());
+}
+
+#[test]
+fn test_validate_ref_name_rejects_dot_prefix_suffix() {
+    assert!(validate_ref_name(".branch").is_err());
+    assert!(validate_ref_name("branch.").is_err());
+    assert!(validate_ref_name(".branch.").is_err());
+}
+
+#[test]
+fn test_validate_ref_name_rejects_slash_suffix() {
+    assert!(validate_ref_name("branch/").is_err());
+}
+
+#[test]
+fn test_validate_ref_name_rejects_double_dot() {
+    assert!(validate_ref_name("feature..branch").is_err());
+    assert!(validate_ref_name("..branch").is_err());
+    assert!(validate_ref_name("branch..").is_err());
+}
+
+#[test]
+fn test_validate_ref_name_rejects_at_brace() {
+    assert!(validate_ref_name("branch@{1}").is_err());
+    assert!(validate_ref_name("@{upstream}").is_err());
+    assert!(validate_ref_name("feature/@{branch}").is_err());
+}
+
+#[test]
+fn test_validate_ref_name_rejects_forbidden_chars() {
+    assert!(validate_ref_name("branch~1").is_err());
+    assert!(validate_ref_name("branch^1").is_err());
+    assert!(validate_ref_name("branch:feature").is_err());
+    assert!(validate_ref_name("branch?query").is_err());
+    assert!(validate_ref_name("branch*glob").is_err());
+    assert!(validate_ref_name("branch[0]").is_err());
+    assert!(validate_ref_name("branch\\escape").is_err());
+    assert!(validate_ref_name("branch name").is_err());
+    assert!(validate_ref_name("branch\ttab").is_err());
+    assert!(validate_ref_name("branch\nnewline").is_err());
+    assert!(validate_ref_name("branch\rreturn").is_err());
+    assert!(validate_ref_name("branch\0null").is_err());
+}
+
+#[test]
+fn test_validate_ref_name_rejects_too_long() {
+    let long_name = "a".repeat(256);
+    assert!(validate_ref_name(&long_name).is_err());
+
+    let max_valid_name = "a".repeat(255);
+    assert!(validate_ref_name(&max_valid_name).is_ok());
+}
+
+// ==================== validate_revision tests ====================
+
+#[test]
+fn test_validate_revision_accepts_empty() {
+    assert!(validate_revision("").is_err());
+}
+
+#[test]
+fn test_validate_revision_accepts_head() {
+    assert!(validate_revision("HEAD").is_ok());
+}
+
+#[test]
+fn test_validate_revision_accepts_valid_hex() {
+    assert!(validate_revision("abc1234").is_ok());
+    assert!(validate_revision("abc1234567890123456789012345678901234567890").is_ok());
+    assert!(validate_revision("deadbeef").is_ok());
+}
+
+#[test]
+fn test_validate_revision_rejects_invalid_hex_length() {
+    // "abc" is 3 chars - too short to be hex OID (requires 4-64), but valid as branch name
+    assert!(validate_revision("abc").is_ok());
+
+    let too_long = "a".repeat(65);
+    // 65 hex chars - too long to be hex OID, but might be valid as branch name
+    // However, it will fail ref name length check (> 255 chars would fail, but 65 is fine)
+    // Actually 65 chars of 'a' is a valid branch name, so it should pass
+    assert!(validate_revision(&too_long).is_ok());
+}
+
+#[test]
+fn test_validate_revision_accepts_ref_prefix() {
+    assert!(validate_revision("ref:refs/heads/main").is_ok());
+    assert!(validate_revision("ref:refs/tags/v1.0.0").is_ok());
+}
+
+#[test]
+fn test_validate_revision_accepts_ancestry_operators() {
+    assert!(validate_revision("main~1").is_ok());
+    assert!(validate_revision("main~10").is_ok());
+    assert!(validate_revision("HEAD~10000").is_ok());
+    assert!(validate_revision("main^1").is_ok());
+    assert!(validate_revision("main^2").is_ok());
+    assert!(validate_revision("HEAD^10000").is_ok());
+}
+
+#[test]
+fn test_validate_revision_rejects_excessive_depth() {
+    assert!(validate_revision("main~10001").is_err());
+    assert!(validate_revision("main~999999999999").is_err());
+    assert!(validate_revision("main^10001").is_err());
+    assert!(validate_revision("main^999999999999").is_err());
+}
+
+#[test]
+fn test_validate_revision_accepts_tree_suffix() {
+    assert!(validate_revision("main^{tree}").is_ok());
+    assert!(validate_revision("HEAD^{tree}").is_ok());
+}
+
+#[test]
+fn test_validate_revision_rejects_too_long() {
+    // Test length limit (256 chars) - use a simple branch name to avoid depth checks
+    let long_rev = "a".repeat(257);
+    assert!(validate_revision(&long_rev).is_err());
+
+    // For non-hex revisions, the effective limit is 255 chars (ref name limit)
+    let max_valid = "a".repeat(255);
+    assert!(validate_revision(&max_valid).is_ok());
+}
+
+#[test]
+fn test_validate_revision_accepts_valid_branch_names() {
+    assert!(validate_revision("main").is_ok());
+    assert!(validate_revision("feature/new-api").is_ok());
+    // v1.0.0 contains dots but they're not at start/end, so it's valid
+    assert!(validate_revision("v1.0.0").is_ok());
+}
+
+// ==================== validate_file_path tests ====================
+
+#[test]
+fn test_validate_file_path_accepts_valid_paths() {
+    assert!(validate_file_path("file.txt").is_ok());
+    assert!(validate_file_path("src/main.rs").is_ok());
+    assert!(validate_file_path("deep/nested/path/file.js").is_ok());
+    assert!(validate_file_path("README.md").is_ok());
+}
+
+#[test]
+fn test_validate_file_path_rejects_empty() {
+    assert!(validate_file_path("").is_err());
+}
+
+#[test]
+fn test_validate_file_path_rejects_absolute_paths() {
+    assert!(validate_file_path("/etc/passwd").is_err());
+    assert!(validate_file_path("/absolute/path").is_err());
+    assert!(validate_file_path("/file.txt").is_err());
+}
+
+#[test]
+fn test_validate_file_path_rejects_path_traversal() {
+    assert!(validate_file_path("../escape").is_err());
+    assert!(validate_file_path("path/../escape").is_err());
+    assert!(validate_file_path("path/../../escape").is_err());
+    assert!(validate_file_path("..").is_err());
+}
+
+#[test]
+fn test_validate_file_path_rejects_null_bytes() {
+    assert!(validate_file_path("file\0.txt").is_err());
+    assert!(validate_file_path("path/\0escape").is_err());
+}
+
+#[test]
+fn test_validate_file_path_rejects_git_directory() {
+    assert!(validate_file_path(".git").is_err());
+    assert!(validate_file_path(".git/config").is_err());
+    assert!(validate_file_path(".git/hooks/pre-commit").is_err());
+    assert!(validate_file_path("path/.git/config").is_err());
+    assert!(validate_file_path(".git/").is_err());
+}
+
+#[test]
+fn test_validate_file_path_rejects_too_long() {
+    let long_path = "a".repeat(4097);
+    assert!(validate_file_path(&long_path).is_err());
+
+    let max_valid_path = "a".repeat(4096);
+    assert!(validate_file_path(&max_valid_path).is_ok());
+}
+
+#[cfg(windows)]
+#[test]
+fn test_validate_file_path_rejects_windows_reserved_names() {
+    assert!(validate_file_path("CON").is_err());
+    assert!(validate_file_path("PRN").is_err());
+    assert!(validate_file_path("AUX").is_err());
+    assert!(validate_file_path("NUL").is_err());
+    assert!(validate_file_path("COM1").is_err());
+    assert!(validate_file_path("LPT1").is_err());
+    assert!(validate_file_path("path/CON").is_err());
+    assert!(validate_file_path("CON.txt").is_err());
+}
+
+// ==================== validate_relative_path tests ====================
+
+#[test]
+fn test_validate_relative_path_accepts_valid_paths() {
+    assert!(validate_relative_path("repo").is_ok());
+    assert!(validate_relative_path("path/to/repo").is_ok());
+    assert!(validate_relative_path("user/project").is_ok());
+}
+
+#[test]
+fn test_validate_relative_path_rejects_empty() {
+    assert!(validate_relative_path("").is_err());
+}
+
+#[test]
+fn test_validate_relative_path_rejects_absolute() {
+    assert!(validate_relative_path("/absolute/path").is_err());
+    assert!(validate_relative_path("/etc").is_err());
+}
+
+#[test]
+fn test_validate_relative_path_rejects_traversal() {
+    assert!(validate_relative_path("../escape").is_err());
+    assert!(validate_relative_path("path/../escape").is_err());
+    assert!(validate_relative_path("..").is_err());
+    assert!(validate_relative_path("path/..").is_err());
+}
+
+// ==================== validate_config_key tests ====================
+
+#[test]
+fn test_validate_config_key_accepts_safe_keys() {
+    assert!(validate_config_key("user.name").is_ok());
+    assert!(validate_config_key("user.email").is_ok());
+    assert!(validate_config_key("core.editor").is_ok());
+    assert!(validate_config_key("alias.co").is_ok());
+}
+
+#[test]
+fn test_validate_config_key_rejects_empty() {
+    assert!(validate_config_key("").is_err());
+}
+
+#[test]
+fn test_validate_config_key_rejects_dangerous_keys() {
+    assert!(validate_config_key("core.sshCommand").is_err());
+    assert!(validate_config_key("core.hooksPath").is_err());
+    assert!(validate_config_key("safe.directory").is_err());
+}
+
+#[test]
+fn test_validate_config_key_rejects_wildcard_dangerous_keys() {
+    assert!(validate_config_key("remote.origin.url").is_err());
+    assert!(validate_config_key("remote.upstream.url").is_err());
+    assert!(validate_config_key("http.proxy").is_err());
+    assert!(validate_config_key("https.proxy").is_err());
+}
+
+#[test]
+fn test_validate_config_key_rejects_invalid_chars() {
+    assert!(validate_config_key("key with space").is_err());
+    assert!(validate_config_key("key;rm -rf").is_err());
+    assert!(validate_config_key("key$(command)").is_err());
+    assert!(validate_config_key("key`command`").is_err());
+}