fix(controller): allowlist module names to close command injection (D2)

CLAUDE.md

@@ -159,6 +159,21 @@ validation, allowing config-directive/URL injection.
   `RsyncManager` re-checks defensively before writing the daemon config / building
   the client URL. Reusable by the remaining injection fixes (S4, D2) as they migrate.
 
+**Slice (DONE) — deploy module-name allowlist (`org.iiab.controller.deploy.domain`)**
+Phase-1 security slice closing tech-debt **D2** (command injection). The install
+queue interpolates a module name into a `sed/echo/runrole` command run as root
+in the container; the name comes from the fixed `ModuleRegistry` catalog but
+round-trips through SharedPreferences.
+
+- `domain/` — `ModuleName` (pure JVM): `isAllowed(name, knownKeys)` = the name is
+  a known catalog key AND matches `[a-z0-9_-]` (no shell metacharacters).
+  Unit-tested (`deploy/domain/ModuleNameTest`).
+- `ModuleRegistry.validYamlKeys()` is the single allowlist source (derived from
+  `MASTER_ROSTER`).
+- **Legacy seam:** `DeployFragment.processNextInQueue()` validates the popped
+  module and fails closed (logs + skips) before building the command. The command
+  structure is unchanged for legitimate modules.
+
 **Legacy (NOT yet layered)** — most of `org.iiab.controller` is still flat:
 god classes `MainActivity` and `DeployFragment` (~2.7k LOC), shared mutable
 state on public/static fields, hand-rolled `HttpURLConnection` calls duplicated

controller/app/src/main/java/org/iiab/controller/DeployFragment.java

@@ -8,6 +8,7 @@
  */
 package org.iiab.controller;
 
+import org.iiab.controller.deploy.domain.ModuleName;
 import android.app.NotificationChannel;
 import android.app.NotificationManager;
 import android.app.PendingIntent;
@@ -18,6 +19,7 @@
 import android.content.res.ColorStateList;
 import android.graphics.Color;
 import android.graphics.Typeface;
+import android.util.Log;
 import android.net.Uri;
 import android.os.Bundle;
 import android.os.Environment;
@@ -1393,6 +1395,19 @@ private void processNextInQueue() {
 
         String nextModule = installationQueue.remove(0);
         saveQueueToPrefs();
+
+        // D2: nextModule is interpolated into a command run as root inside the
+        // container (sed/echo/runrole). Only allow names from the known catalog
+        // with no shell metacharacters; fail closed and skip anything else.
+        if (!ModuleName.isAllowed(nextModule, ModuleRegistry.validYamlKeys())) {
+            Log.e(TAG, "Refusing to install unrecognized/unsafe module name: " + nextModule);
+            if (getActivity() instanceof MainActivity) {
+                ((MainActivity) getActivity()).addToLog("[Security] Skipped invalid module: " + nextModule);
+            }
+            processNextInQueue();
+            return;
+        }
+
         btnLaunchInstall.setEnabled(false);
         btnLaunchInstall.setText(getString(R.string.install_status_installing_module, nextModule));
 

controller/app/src/main/java/org/iiab/controller/ModuleRegistry.java

@@ -10,7 +10,9 @@
 package org.iiab.controller;
 
 import java.util.Arrays;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
 
 public class ModuleRegistry {
 
@@ -41,4 +43,17 @@ public IiabModule(String endpoint, int nameResId, boolean requires64Bit, String
             // so the system doesn't break if it gets added in the future.
             new IiabModule("dashboard", R.string.dash_system, false, "dashboard")
     );
-}
+
+    /**
+     * The set of valid module YAML keys. This is the single allowlist for any
+     * value interpolated into a shell/Ansible command (see D2). Derived from
+     * {@link #MASTER_ROSTER} so the catalog stays the single source of truth.
+     */
+    public static Set<String> validYamlKeys() {
+        Set<String> keys = new HashSet<>();
+        for (IiabModule m : MASTER_ROSTER) {
+            keys.add(m.yamlBaseKey);
+        }
+        return keys;
+    }
+}

controller/app/src/main/java/org/iiab/controller/deploy/domain/ModuleName.java

@@ -0,0 +1,67 @@
+/*
+ * ============================================================================
+ * Name        : ModuleName.java
+ * Author      : IIAB Project
+ * Copyright   : Copyright (c) 2026 IIAB Project
+ * Description : Domain rule: a module name is only safe to install if it is a
+ *               known catalog entry AND contains no shell metacharacters.
+ *               Closes tech-debt item D2 (command injection via module name).
+ * ============================================================================
+ */
+package org.iiab.controller.deploy.domain;
+
+import java.util.Set;
+
+/**
+ * Pure (framework-free) guard for module names that get interpolated into a
+ * shell/Ansible command executed <strong>as root inside the container</strong>
+ * (see {@code DeployFragment}: {@code sed ... && echo ... && ./runrole NAME}).
+ *
+ * <p>A crafted name containing a quote, {@code ;}, {@code &&}, {@code $()} etc.
+ * would break out of the command — tech-debt item <b>D2</b>. Module names come
+ * from a fixed catalog ({@code ModuleRegistry.MASTER_ROSTER}) and round-trip
+ * through SharedPreferences, so this enforces an <em>allowlist</em>: the name
+ * must be one of the known catalog keys and also be well-formed.
+ *
+ * <p>No {@code android.*} here so it is unit-testable on a plain JVM and reusable
+ * by any other code path that interpolates a module name.
+ */
+public final class ModuleName {
+
+    private static final int MAX_LEN = 64;
+
+    private ModuleName() {
+        // Static utility; not instantiable.
+    }
+
+    /**
+     * True if {@code name} is one of the {@code known} catalog keys and is
+     * well-formed. Fail-closed: anything not explicitly allowed is rejected.
+     */
+    public static boolean isAllowed(String name, Set<String> known) {
+        return name != null && known != null && known.contains(name) && isWellFormed(name);
+    }
+
+    /**
+     * True if {@code name} is a safe module identifier: a non-empty run of
+     * {@code [a-z0-9_-]} (no uppercase, whitespace, quotes or shell
+     * metacharacters), at most {@value #MAX_LEN} chars. Matches every key in the
+     * current roster (e.g. {@code calibreweb}, {@code kiwix}, {@code maps}).
+     */
+    public static boolean isWellFormed(String name) {
+        if (name == null || name.isEmpty() || name.length() > MAX_LEN) {
+            return false;
+        }
+        for (int i = 0; i < name.length(); i++) {
+            char c = name.charAt(i);
+            boolean ok = (c >= 'a' && c <= 'z')
+                    || (c >= '0' && c <= '9')
+                    || c == '_'
+                    || c == '-';
+            if (!ok) {
+                return false;
+            }
+        }
+        return true;
+    }
+}

controller/app/src/test/java/org/iiab/controller/deploy/domain/ModuleNameTest.java

@@ -0,0 +1,73 @@
+package org.iiab.controller.deploy.domain;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
+
+import org.junit.Test;
+
+/**
+ * Unit tests for {@link ModuleName} — the allowlist guard that closes D2
+ * (command injection via a module name interpolated into a root shell command).
+ * Pure JVM, no Android dependencies.
+ */
+public class ModuleNameTest {
+
+    private static final Set<String> ROSTER = new HashSet<>(Arrays.asList(
+            "calibreweb", "code", "kiwix", "kolibri", "maps", "matomo", "dashboard"));
+
+    @Test
+    public void acceptsEveryKnownRosterModule() {
+        for (String m : ROSTER) {
+            assertTrue("roster module rejected: " + m, ModuleName.isAllowed(m, ROSTER));
+        }
+    }
+
+    @Test
+    public void rejectsShellInjectionEvenIfWellFormedPrefix() {
+        assertFalse(ModuleName.isAllowed("kiwix; rm -rf /", ROSTER));
+        assertFalse(ModuleName.isAllowed("kiwix' && reboot #", ROSTER));
+        assertFalse(ModuleName.isAllowed("$(reboot)", ROSTER));
+        assertFalse(ModuleName.isAllowed("maps`id`", ROSTER));
+        assertFalse(ModuleName.isAllowed("maps\nmatomo", ROSTER));
+    }
+
+    @Test
+    public void rejectsWellFormedButUnknownModule() {
+        // Charset-valid but not in the catalog -> still rejected (allowlist).
+        assertFalse(ModuleName.isAllowed("evil", ROSTER));
+        assertFalse(ModuleName.isAllowed("wordpress", ROSTER));
+    }
+
+    @Test
+    public void rejectsNulls() {
+        assertFalse(ModuleName.isAllowed(null, ROSTER));
+        assertFalse(ModuleName.isAllowed("kiwix", null));
+    }
+
+    // --- isWellFormed (the charset rule on its own) ---
+
+    @Test
+    public void wellFormedAcceptsCatalogStyleNames() {
+        assertTrue(ModuleName.isWellFormed("calibreweb"));
+        assertTrue(ModuleName.isWellFormed("kiwix"));
+        assertTrue(ModuleName.isWellFormed("a-b_c1"));
+    }
+
+    @Test
+    public void wellFormedRejectsMetacharsAndCase() {
+        assertFalse(ModuleName.isWellFormed("Kiwix"));      // uppercase
+        assertFalse(ModuleName.isWellFormed("a b"));        // space
+        assertFalse(ModuleName.isWellFormed("a;b"));        // semicolon
+        assertFalse(ModuleName.isWellFormed("a'b"));        // quote
+        assertFalse(ModuleName.isWellFormed("a/b"));        // slash
+        assertFalse(ModuleName.isWellFormed(""));           // empty
+        assertFalse(ModuleName.isWellFormed(null));         // null
+        StringBuilder sb = new StringBuilder();
+        for (int i = 0; i < 65; i++) sb.append('a');
+        assertFalse(ModuleName.isWellFormed(sb.toString())); // too long
+    }
+}

controller/docs/TECH_DEBT_PLAN.md

@@ -47,8 +47,12 @@ _Last updated: 2026-06-17. Tracks remediation work against the findings below. I
 - **Integrity**: `--check-integrity=true` makes aria2 verify the SHA-256 checksums published in the `.meta4` (confirmed present: file-level + per-piece) during download; on mismatch aria2 exits non-zero, `onError` fires and the archive is never extracted. Uses the server's published hash + verified TLS, so no redundant on-device re-hash of the ~1.2 GB file.
 - Follow-up (separate items): ZIM content integrity (Kiwix publishes no embedded hash today; needs `.sha256` sidecars) and the cleartext OTA APK path in `MainActivity` (**F15**).
 
-**Phase 1 — Security hardening: IN PROGRESS.** Done so far: **S1** (PR #9), **M4**, **S3** (PR #10), **D6**. Remaining: **D2**, **D12**, **S4**, **D11**.
+**D2 — Module-name command injection (Phase 1 security): DONE** (PR `fix/phase1-security-d2-module-injection`)
+- Closes **D2**: `DeployFragment.processNextInQueue()` interpolated the module name into a `sed ... && echo ... && ./runrole NAME` command executed as root in the container. Names come from the fixed `ModuleRegistry` catalog but round-trip through SharedPreferences, so a tampered/unknown value with shell metacharacters could inject commands.
+- New pure domain rule `org.iiab.controller.deploy.domain.ModuleName.isAllowed(name, known)`: the name must be a known catalog key (allowlist) AND match `[a-z0-9_-]` (no quotes/`;`/`&&`/`$()`). `ModuleRegistry.validYamlKeys()` is the single allowlist source. Unit-tested (`ModuleNameTest`).
+- The guard fails closed: an unrecognized/unsafe module is logged and skipped before the command is built; the command structure is unchanged for legitimate modules. Lower-risk on-device `sh -c` pipes (extract/backup, app-internal paths) are a documented follow-up.
 
+**Phase 1 — Security hardening: IN PROGRESS.** Done so far: **S1** (PR #9), **M4**, **S3** (PR #10), **D6** (PR #12), **D2**. Remaining: **D12**, **S4**, **D11**.
 ## 1. Executive summary
 
 The Controller is functional and shows real security intent (it SHA256-audits native binaries at build time, scrubs the keystore in CI, and scopes most broadcasts). But it carries debt on four fronts that scale badly toward the README's "millions of users" goal: