From 3fd66298862c60cb1af0bbc1424b8ba3e7d582b3 Mon Sep 17 00:00:00 2001
From: Mykhailo Chalyi <mike@chaliy.name>
Date: Thu, 2 Apr 2026 11:12:34 +0000
Subject: [PATCH] fix(builtins): block unset of internal variables and readonly
 marker bypass
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Closes #1006 — unset could remove _READONLY_* marker variables directly,
defeating readonly protection. Now checks is_internal_variable() in the
interpreter's execute_unset_builtin to block removal of all internal
markers. Also adds defense-in-depth checks in the Unset builtin struct.
---
 crates/bashkit/src/builtins/vars.rs           | 26 ++++++++++-
 crates/bashkit/src/interpreter/mod.rs         |  4 ++
 .../bashkit/tests/blackbox_security_tests.rs  | 43 +++++++++++++++++++
 3 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/crates/bashkit/src/builtins/vars.rs b/crates/bashkit/src/builtins/vars.rs
index f84e6d21..42965ada 100644
--- a/crates/bashkit/src/builtins/vars.rs
+++ b/crates/bashkit/src/builtins/vars.rs
@@ -13,13 +13,37 @@ pub struct Unset;
 
 #[async_trait]
 impl Builtin for Unset {
+    // THREAT[TM-INJ-009]: Block unset of internal variables and readonly variables
     async fn execute(&self, ctx: Context<'_>) -> Result<ExecResult> {
+        let mut stderr = String::new();
+        let mut exit_code = 0;
         for name in ctx.args {
+            // Block unsetting internal marker variables (_READONLY_, _NAMEREF_, etc.)
+            if is_internal_variable(name) {
+                stderr.push_str(&format!(
+                    "bash: unset: {name}: cannot unset: readonly variable\n"
+                ));
+                exit_code = 1;
+                continue;
+            }
+            // Block unsetting readonly variables
+            let readonly_key = format!("_READONLY_{name}");
+            if ctx.variables.contains_key(&readonly_key) {
+                stderr.push_str(&format!(
+                    "bash: unset: {name}: cannot unset: readonly variable\n"
+                ));
+                exit_code = 1;
+                continue;
+            }
             ctx.variables.remove(name);
             // Note: env is immutable in our model - environment variables
             // are inherited and can't be unset by the shell
         }
-        Ok(ExecResult::ok(String::new()))
+        Ok(ExecResult {
+            stderr,
+            exit_code,
+            ..Default::default()
+        })
     }
 }
 
diff --git a/crates/bashkit/src/interpreter/mod.rs b/crates/bashkit/src/interpreter/mod.rs
index 44c2e210..c79612c9 100644
--- a/crates/bashkit/src/interpreter/mod.rs
+++ b/crates/bashkit/src/interpreter/mod.rs
@@ -4605,6 +4605,10 @@ impl Interpreter {
                 self.variables.remove(&format!("_NAMEREF_{}", arg));
             } else {
                 let resolved = self.resolve_nameref(arg).to_string();
+                // THREAT[TM-INJ-009]: Block unset of internal marker variables
+                if is_internal_variable(&resolved) {
+                    continue;
+                }
                 // THREAT[TM-INJ-019]: Refuse to unset readonly variables
                 if self
                     .variables
diff --git a/crates/bashkit/tests/blackbox_security_tests.rs b/crates/bashkit/tests/blackbox_security_tests.rs
index 16a02683..a43ea38a 100644
--- a/crates/bashkit/tests/blackbox_security_tests.rs
+++ b/crates/bashkit/tests/blackbox_security_tests.rs
@@ -252,6 +252,49 @@ mod finding_readonly_bypass {
         );
     }
 
+    /// Issue #1006: unset _READONLY_* marker cannot bypass readonly protection.
+    #[tokio::test]
+    async fn unset_readonly_marker_blocked() {
+        let mut bash = tight_bash();
+        let result = bash
+            .exec(
+                r#"
+                readonly IMPORTANT=secret
+                unset _READONLY_IMPORTANT 2>/dev/null
+                IMPORTANT=hacked 2>/dev/null
+                echo "IMPORTANT=$IMPORTANT"
+                "#,
+            )
+            .await
+            .unwrap();
+        assert!(
+            result.stdout.contains("IMPORTANT=secret"),
+            "readonly was bypassed by unsetting _READONLY_ marker, got: {}",
+            result.stdout
+        );
+    }
+
+    /// Unset of normal non-readonly variables still works.
+    #[tokio::test]
+    async fn unset_normal_variable_works() {
+        let mut bash = tight_bash();
+        let result = bash
+            .exec(
+                r#"
+                FOO=hello
+                unset FOO
+                echo "FOO=${FOO:-empty}"
+                "#,
+            )
+            .await
+            .unwrap();
+        assert!(
+            result.stdout.contains("FOO=empty"),
+            "expected FOO to be unset, got: {}",
+            result.stdout
+        );
+    }
+
     /// TM-INJ-020: declare cannot overwrite readonly variables.
     #[tokio::test]
     async fn declare_cannot_overwrite_readonly() {