From 4177b5c50e3e083976e2a918157cbd165ca60d53 Mon Sep 17 00:00:00 2001
From: krypticmouse <herumbshandilya123@gmail.com>
Date: Wed, 20 May 2026 03:36:28 +0000
Subject: [PATCH] fix(skills): satisfy Clippy + reject non-ASCII hex + add
 regression tests
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Follow-up on @tomaioo's signature-verification panic fix in PR #235.

1. Clippy on Rust 1.95+ flags `len() % 2 != 0` with the
   `manual_is_multiple_of` lint, which was failing the `rust` CI job
   on PR #235 and blocking merge. Switch to `is_multiple_of(2)`.

2. Add an explicit `is_ascii()` guard before slicing. With only the
   length check, a non-ASCII input (e.g. `"é"` — 2 bytes but 1 char)
   would survive the length check before `from_str_radix` caught it.
   The explicit guard makes the rejection intent clear and avoids
   relying on the post-slice error path.

3. Extract the hex-parsing into a private `parse_public_key_hex`
   helper. PyO3-bound `#[pymethods]` are awkward to unit-test from
   Rust (need a Python interpreter via `prepare_freethreaded_python`);
   a plain function is testable with no GIL boilerplate.

4. Add 5 unit tests covering the security boundary:
   - empty input -> Some(empty vec)
   - valid hex decodes to the right bytes
   - odd-length rejected without panic (the original bug)
   - non-hex chars rejected (previously silently filtered)
   - multi-byte UTF-8 rejected without panic

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
 rust/crates/openjarvis-python/src/skills.rs | 80 +++++++++++++++++----
 1 file changed, 68 insertions(+), 12 deletions(-)

diff --git a/rust/crates/openjarvis-python/src/skills.rs b/rust/crates/openjarvis-python/src/skills.rs
index 7f9cea4a..940c43d7 100644
--- a/rust/crates/openjarvis-python/src/skills.rs
+++ b/rust/crates/openjarvis-python/src/skills.rs
@@ -48,25 +48,81 @@ impl PySkillManifest {
     }
 
     fn verify_signature(&self, public_key_hex: &str) -> bool {
-        if public_key_hex.len() % 2 != 0 {
-            return false;
+        match parse_public_key_hex(public_key_hex) {
+            Some(key_bytes) => openjarvis_skills::verify_signature(&self.inner, &key_bytes),
+            None => false,
         }
-
-        let mut key_bytes = Vec::with_capacity(public_key_hex.len() / 2);
-        for i in (0..public_key_hex.len()).step_by(2) {
-            match u8::from_str_radix(&public_key_hex[i..i + 2], 16) {
-                Ok(byte) => key_bytes.push(byte),
-                Err(_) => return false,
-            }
-        }
-
-        openjarvis_skills::verify_signature(&self.inner, &key_bytes)
     }
 }
 
+fn parse_public_key_hex(public_key_hex: &str) -> Option<Vec<u8>> {
+    if !public_key_hex.len().is_multiple_of(2) {
+        return None;
+    }
+    if !public_key_hex.is_ascii() {
+        return None;
+    }
+
+    let mut key_bytes = Vec::with_capacity(public_key_hex.len() / 2);
+    for i in (0..public_key_hex.len()).step_by(2) {
+        match u8::from_str_radix(&public_key_hex[i..i + 2], 16) {
+            Ok(byte) => key_bytes.push(byte),
+            Err(_) => return None,
+        }
+    }
+    Some(key_bytes)
+}
+
 #[pyfunction]
 pub fn load_skill(toml_str: &str) -> PyResult<PySkillManifest> {
     let manifest = openjarvis_skills::load_skill(toml_str)
         .map_err(|e| PyErr::new::<pyo3::exceptions::PyValueError, _>(e))?;
     Ok(PySkillManifest { inner: manifest })
 }
+
+#[cfg(test)]
+mod tests {
+    use super::parse_public_key_hex;
+
+    #[test]
+    fn empty_input_returns_empty_vec() {
+        assert_eq!(parse_public_key_hex(""), Some(Vec::new()));
+    }
+
+    #[test]
+    fn valid_hex_decodes() {
+        assert_eq!(
+            parse_public_key_hex("0a1b2cFF"),
+            Some(vec![0x0a, 0x1b, 0x2c, 0xff])
+        );
+    }
+
+    #[test]
+    fn odd_length_rejected_without_panic() {
+        // Regression: the pre-fix implementation sliced public_key_hex[i..i+2]
+        // on an odd-length string, triggering an out-of-bounds panic and a
+        // DoS vector when the input was attacker-controlled.
+        assert_eq!(parse_public_key_hex("0"), None);
+        assert_eq!(parse_public_key_hex("abc"), None);
+        assert_eq!(parse_public_key_hex("0a1b2"), None);
+    }
+
+    #[test]
+    fn non_hex_chars_rejected() {
+        // Pre-fix `filter_map` silently dropped non-hex chars and produced a
+        // truncated key, which would also have caused verification surprises.
+        assert_eq!(parse_public_key_hex("zz"), None);
+        assert_eq!(parse_public_key_hex("0aZZ"), None);
+        assert_eq!(parse_public_key_hex("gh"), None);
+    }
+
+    #[test]
+    fn multibyte_utf8_rejected_without_panic() {
+        // Pre-fix indexing public_key_hex[i..i+2] on a non-ASCII string could
+        // split a multi-byte UTF-8 codepoint and panic. The ASCII-only check
+        // makes the rejection explicit instead of relying on from_str_radix's
+        // post-slice error path.
+        assert_eq!(parse_public_key_hex("é"), None);
+        assert_eq!(parse_public_key_hex("aaé"), None);
+    }
+}