WIP: Test skill

.github/actions/ci/action.yml

@@ -19,6 +19,7 @@ runs:
           ~/.dylint_drivers/
           ~/.rustup/toolchains/
           target/dylint/
+          target/necessist-audit-cache/
         key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }}
 
     - uses: taiki-e/install-action@v2.85.10
@@ -45,6 +46,24 @@ runs:
       if: ${{ env.TESTS != 'ci' }}
       uses: ./.github/actions/install-testing-tools
 
+    - name: Install LLM tools
+      if: ${{ (runner.os == 'Linux' || runner.os == 'macOS') && contains(env.TESTS, 'necessist_audit') }}
+      run: |
+        curl -fsSL https://claude.ai/install.sh | bash
+        curl -fsSL https://chatgpt.com/codex/install.sh | sh
+        printenv OPENAI_API_KEY | codex login --with-api-key
+      shell: bash
+
+    # smoelius: Codex requires Bubblewrap.
+    - name: Install Bubblewrap on Ubuntu
+      if: ${{ runner.os == 'Linux' && contains(env.TESTS, 'necessist_audit') }}
+      run: |
+        sudo apt install bubblewrap
+        sudo apt install apparmor-profiles
+        sudo cp /usr/share/apparmor/extra-profiles/bwrap-userns-restrict /etc/apparmor.d || true
+        sudo systemctl reload apparmor
+      shell: bash
+
     - name: Install sqlite3 on Ubuntu
       if: ${{ runner.os == 'Linux' }}
       run: |
@@ -82,6 +101,11 @@ runs:
       run: |
         for TEST in $TESTS; do
           if [[ "$TEST" != 'other' ]]; then
+            if [[ "$TEST" = 'necessist_audit' && '${{ github.event_name }}' != 'schedule' && '${{ github.event_name }}' != 'workflow_dispatch' ]] &&
+              ! git diff --name-only ${{ github.event.pull_request.base.sha }} | grep 'necessist_audit' >/dev/null
+            then
+              exit 0
+            fi
             $CARGO_TEST -p necessist --test "$TEST"
           else
             $CARGO_TEST -p necessist --test general

.github/workflows/ci.yml

@@ -35,7 +35,7 @@ jobs:
       fail-fast: ${{ github.event_name == 'merge_group' }}
       matrix:
         environment: [ubuntu-latest, macos-latest, windows-latest]
-        test: [third_party_0, third_party_1, trycmd, other]
+        test: [necessist_audit, third_party_0, third_party_1, trycmd, other]
         include:
           - environment: ubuntu-latest
             test: ci
@@ -48,6 +48,8 @@ jobs:
       - uses: ./.github/actions/ci
         env:
           TESTS: ${{ matrix.test }}
+          ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }}
+          OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}
 
   all-checks:
     needs: [test]

necessist/tests/necessist_audit.rs

@@ -0,0 +1,100 @@
+//! A paper on Necessist (Test Harness Mutilation) appeared in Mutation 2024.
+//!
+//! This test audits one of its Go standard library findings.
+//!
+//! The structured-report approach is inspired by the worker finding artifacts in
+//! the Trail of Bits `c-review` skill:
+//! <https://github.com/trailofbits/skills/tree/main/plugins/c-review>
+
+use assert_cmd::output::OutputError;
+use necessist_core::util;
+use std::{
+    env::{split_paths, var_os},
+    io::{Write, stderr},
+    path::{Path, PathBuf},
+    process::{Command, Output},
+    time::Duration,
+};
+
+mod necessist_audit_support;
+use necessist_audit_support::tool::Tool;
+
+const GO_REPO: &str = "https://github.com/golang/go";
+const GO_REV: &str = "9a0a82445650eebedf5633fdfe6e73b5836dc5c9";
+const JSON_REPORT: &str = "necessist-audit.json";
+const OUTPUT_POLL_INTERVAL: Duration = Duration::from_millis(100);
+const TIMEOUT: Duration = Duration::from_mins(10);
+
+#[test]
+#[cfg_attr(dylint_lib = "general", allow(non_thread_safe_call_in_test))]
+fn necessist_audit_claude() {
+    let Some(tool_path) = command_on_path("claude") else {
+        #[allow(clippy::explicit_write)]
+        writeln!(
+            stderr(),
+            "Skipping `necessist_audit_claude` because `claude` is not on PATH"
+        )
+        .unwrap();
+        return;
+    };
+
+    Tool::claude(tool_path).run();
+}
+
+#[test]
+#[cfg_attr(dylint_lib = "general", allow(non_thread_safe_call_in_test))]
+fn necessist_audit_codex() {
+    let Some(tool_path) = command_on_path("codex") else {
+        #[allow(clippy::explicit_write)]
+        writeln!(
+            stderr(),
+            "Skipping `necessist_audit_codex` because `codex` is not on PATH"
+        )
+        .unwrap();
+        return;
+    };
+
+    Tool::codex(tool_path).run();
+}
+
+fn command_on_path(tool: &str) -> Option<PathBuf> {
+    let path_var = var_os("PATH")?;
+    split_paths(&path_var).find_map(|dir| {
+        let candidate = dir.join(tool);
+        candidate.is_file().then_some(candidate)
+    })
+}
+
+fn command_output(command: &mut Command) -> Output {
+    let output = command.output().unwrap();
+    assert!(
+        output.status.success(),
+        "command failed\n{}",
+        OutputError::new(output.clone())
+    );
+    output
+}
+
+fn format_output(output: &Output) -> String {
+    OutputError::new(output.clone()).to_string()
+}
+
+#[cfg_attr(dylint_lib = "supplementary", allow(abs_home_path))]
+fn workspace_root() -> PathBuf {
+    Path::new(env!("CARGO_MANIFEST_DIR"))
+        .parent()
+        .unwrap()
+        .to_owned()
+}
+
+fn skill_dir() -> PathBuf {
+    workspace_root().join("skills/necessist-audit")
+}
+
+fn cache_dir() -> PathBuf {
+    workspace_root().join("target/necessist-audit-cache")
+}
+
+fn display_path(path: &Path) -> &Path {
+    util::strip_prefix(path, &workspace_root()).unwrap_or(path)
+}

necessist/tests/necessist_audit_support/accept.rs

@@ -0,0 +1,169 @@
+use serde_json::Value;
+
+type IsAcceptableResultFn = fn(&Value) -> bool;
+
+pub(crate) fn contains_acceptable_result(findings: &[Value], leads: &[Value]) -> bool {
+    let acceptable_result_fns: &[IsAcceptableResultFn] = &[
+        is_acceptable_done_result,
+        is_acceptable_hello_result,
+        is_acceptable_crlf_oracle_result,
+    ];
+
+    [findings, leads].into_iter().any(|results| {
+        acceptable_result_fns
+            .iter()
+            .any(|&is_acceptable_result| results.iter().any(is_acceptable_result))
+    })
+}
+
+// The paper highlighted this `<-done` removal.
+fn is_acceptable_done_result(value: &Value) -> bool {
+    let removed_code = value
+        .get("removed_code")
+        .and_then(Value::as_str)
+        .unwrap_or_default();
+    if !removed_code.contains("<-done") {
+        return false;
+    }
+
+    assert_json_string_contains(value, "removed_code", "<-done");
+    assert_json_string_contains(value, "removed_location", "smtp_test.go");
+    assert_json_string_contains(value, "affected_location", "smtp_test.go");
+
+    let text = json_text(value);
+    assert!(
+        text.contains("traffic") || text.contains("SMTP") || text.contains("goroutine"),
+        "finding did not include SMTP traffic/goroutine details: {value:#}"
+    );
+    true
+}
+
+// Current LLMs may also report `Hello("customhost")` in `TestHello` case 8:
+// https://github.com/golang/go/blob/9a0a82445650eebedf5633fdfe6e73b5836dc5c9/src/net/smtp/smtp_test.go#L474-L481
+// That is also a valid test-harness finding, though it was not called out in the
+// original paper.
+//
+// The test appears intended to check `Client.Hello`'s documented ordering
+// contract: `Hello` must be called before any other method, and the implementation
+// rejects late calls once `didHello` is set:
+// https://github.com/golang/go/blob/9a0a82445650eebedf5633fdfe6e73b5836dc5c9/src/net/smtp/smtp.go#L96-L105
+//
+// ```go
+// err = c.Verify("test@example.com")
+// if err != nil {
+//     err = c.Hello("customhost")
+//     if err != nil {
+//         t.Errorf("Want error, got none")
+//     }
+// }
+// ```
+//
+// In the fixture, `Verify` succeeds, so the branch is unreachable and the
+// `Hello` call is never exercised. Removing it therefore leaves the test passing
+// and exposes that the harness would not catch a regression where `Hello`
+// incorrectly succeeds after another method. The intended shape is more like:
+//
+// ```go
+// if err := c.Verify("test@example.com"); err != nil {
+//     t.Fatalf("Verify failed: %v", err)
+// }
+// if err := c.Hello("customhost"); err == nil {
+//     t.Errorf("Want error, got none")
+// }
+// ```
+fn is_acceptable_hello_result(value: &Value) -> bool {
+    if !json_text(value).contains("Hello(\"customhost\")") {
+        return false;
+    }
+
+    assert_json_string_contains(value, "removed_code", "Hello(\"customhost\")");
+    assert_json_string_contains(value, "removed_location", "smtp_test.go");
+    assert_json_string_contains(value, "affected_location", "smtp_test.go");
+
+    let text = json_text(value);
+    assert!(
+        text.contains("Verify") && text.contains("Hello"),
+        "finding did not include Verify/Hello details: {value:#}"
+    );
+    true
+}
+
+// Current LLMs may also report the `TestBasic` CR/LF injection checks:
+// https://github.com/golang/go/blob/9a0a82445650eebedf5633fdfe6e73b5836dc5c9/src/net/smtp/smtp_test.go#L197-L217
+// Those are also valid test-harness findings, though they were not called out in
+// the original paper.
+//
+// The test appears intended to check that `Client.Verify`, `Client.Rcpt`, and
+// `Client.Mail` reject CR/LF command injection before sending SMTP commands:
+// https://github.com/golang/go/blob/9a0a82445650eebedf5633fdfe6e73b5836dc5c9/src/net/smtp/smtp.go#L185-L188
+// https://github.com/golang/go/blob/9a0a82445650eebedf5633fdfe6e73b5836dc5c9/src/net/smtp/smtp.go#L246-L249
+// https://github.com/golang/go/blob/9a0a82445650eebedf5633fdfe6e73b5836dc5c9/src/net/smtp/smtp.go#L266-L269
+//
+// ```go
+// if err := c.Verify("user2@gmail.com>\r\nDATA\r\nAnother injected message body\r\n.\r\nQUIT\r\n"); err == nil {
+//     t.Fatalf("VRFY should have failed due to a message injection attempt")
+// }
+// if err := c.Rcpt("golang-nuts@googlegroups.com>\r\nDATA\r\nInjected message body\r\n.\r\nQUIT\r\n"); err == nil {
+//     t.Fatalf("RCPT should have failed due to a message injection attempt")
+// }
+// if err := c.Mail("user@gmail.com>\r\nDATA\r\nAnother injected message body\r\n.\r\nQUIT\r\n"); err == nil {
+//     t.Fatalf("MAIL should have failed due to a message injection attempt")
+// }
+// ```
+//
+// Necessist can remove the selector-call suffix from those short initializer
+// expressions. The remaining initializer is the non-`nil` client `c`, so
+// `err == nil` is false, the failure branch is skipped, and the malicious input
+// is never passed to `Verify`, `Rcpt`, or `Mail`. The SMTP implementation still
+// calls `validateLine`; the defect is that the test oracle can pass without
+// exercising those security checks.
+fn is_acceptable_crlf_oracle_result(value: &Value) -> bool {
+    let removed_code = value
+        .get("removed_code")
+        .and_then(Value::as_str)
+        .unwrap_or_default();
+    if !["Verify", "Rcpt", "Mail"]
+        .iter()
+        .any(|method| removed_code.contains(&format!(".{method}(\"")))
+    {
+        return false;
+    }
+
+    assert_json_string_contains(value, "removed_code", "\\r\\n");
+    assert_json_string_contains(value, "removed_location", "smtp_test.go");
+    assert_json_string_contains(value, "affected_location", "smtp_test.go");
+
+    let text = json_text(value);
+    assert!(
+        (text.contains("CR/LF") || text.contains("injection"))
+            && (text.contains("Verify") || text.contains("Rcpt") || text.contains("Mail")),
+        "finding did not include CR/LF injection oracle details: {value:#}"
+    );
+    true
+}
+
+fn assert_json_string_contains(value: &Value, key: &str, needle: &str) {
+    let haystack = value
+        .get(key)
+        .and_then(Value::as_str)
+        .unwrap_or_else(|| panic!("finding missing string field `{key}`"));
+    assert!(
+        haystack.contains(needle),
+        "expected `{key}` to contain `{needle}`, got `{haystack}`"
+    );
+}
+
+fn json_text(value: &Value) -> String {
+    match value {
+        Value::Null => String::new(),
+        Value::Bool(bool) => bool.to_string(),
+        Value::Number(number) => number.to_string(),
+        Value::String(string) => string.clone(),
+        Value::Array(array) => array.iter().map(json_text).collect::<Vec<_>>().join("\n"),
+        Value::Object(object) => object
+            .values()
+            .map(json_text)
+            .collect::<Vec<_>>()
+            .join("\n"),
+    }
+}