fix(skills): satisfy Clippy + reject non-ASCII hex + add regression tests

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>
This commit is contained in:
krypticmouse
2026-05-20 03:36:28 +00:00
co-authored by Claude Opus 4.7
parent f21eec6c86
commit 4177b5c50e
+68 -12
View File
@@ -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);
}
}