copilot review of security and tests

Author: anildash

.github/workflows/ci.yml

@@ -0,0 +1,72 @@
+name: CI
+
+on:
+  push:
+    branches: [ main ]
+  pull_request:
+  workflow_dispatch: {}
+
+jobs:
+  unit:
+    name: Unit tests (fast)
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - name: Set up Python
+        uses: actions/setup-python@v4
+        with:
+          python-version: '3.11'
+      - name: Show environment
+        run: |
+          python --version
+          uname -a
+          pwd
+          ls -la
+      - name: Cache pip
+        uses: actions/cache@v4
+        with:
+          path: ~/.cache/pip
+          key: ${{ runner.os }}-pip-${{ hashFiles('requirements.txt') }}
+          restore-keys: |
+            ${{ runner.os }}-pip-
+      - name: Install runtime/test deps
+        run: |
+          python -m pip install --upgrade pip
+          pip install -r requirements.txt
+      - name: Run unit tests (skip integration)
+        run: |
+          cd helper
+          PYTHONPATH=. pytest -q -m "not integration"
+
+  integration:
+    name: Integration tests (slow)
+    needs: unit
+    runs-on: ubuntu-latest
+    if: ${{ github.event_name == 'workflow_dispatch' || (github.event_name == 'push' && github.ref == 'refs/heads/main') }}
+    steps:
+      - uses: actions/checkout@v4
+      - name: Set up Python
+        uses: actions/setup-python@v4
+        with:
+          python-version: '3.11'
+      - name: Show environment
+        run: |
+          python --version
+          uname -a
+          pwd
+          ls -la
+      - name: Cache pip
+        uses: actions/cache@v4
+        with:
+          path: ~/.cache/pip
+          key: ${{ runner.os }}-pip-${{ hashFiles('requirements.txt') }}
+          restore-keys: |
+            ${{ runner.os }}-pip-
+      - name: Install runtime/test deps
+        run: |
+          python -m pip install --upgrade pip
+          pip install -r requirements.txt
+      - name: Run integration tests
+        run: |
+          cd helper
+          PYTHONPATH=. pytest -q -m integration

CONTRIBUTING.md

@@ -0,0 +1,40 @@
+Contributing
+============
+
+Quick developer guide for running tests and understanding CI.
+
+Running tests locally
+---------------------
+It's recommended to use a virtual environment for Python work. From the repository root:
+
+- Create and activate a venv (macOS/Linux):
+  python -m venv .venv
+  source .venv/bin/activate
+
+- Install test dependencies:
+  pip install --upgrade pip
+  pip install pytest pyserial cryptography
+
+- Run unit tests only (fast):
+  cd helper
+  pytest -q -m "not integration"
+
+- Run integration tests (slower, network-like):
+  cd helper
+  pytest -q -m integration
+
+- Run the full test suite:
+  cd helper
+  pytest -q
+
+Test markers
+------------
+The test suite separates fast unit tests from slower integration tests using a pytest marker named "integration". Unit/test runs exclude integration tests by default in CI; use the -m flag shown above to include or exclude them locally.
+
+GitHub Actions CI
+-----------------
+The repository includes a GitHub Actions workflow at .github/workflows/ci.yml that runs a fast "unit" job on PRs and pushes, and an "integration" job (dependent on unit) that runs only on main pushes or when manually triggered. The CI caches pip downloads to speed up repeated runs.
+
+If you add new Python test dependencies, update the install steps in the workflow or add a requirements file at the repository root for a more stable cache key.
+
+If you want help adding more information to this guide (e.g., contributing style, commit message conventions, or how to run tests on macOS runners), say which section to expand.

helper/dashtouch_helper/cli.py

@@ -163,6 +163,15 @@ def find_and_flash(prompt_prefix: str = "") -> str:
                         str(REPO / "firmware" / "dashtouch")], check=True)
         subprocess.run(["arduino-cli", "upload", "--fqbn", FQBN, "-p", port,
                         str(REPO / "firmware" / "dashtouch")], check=True)
+        # Erase the plaintext secrets file on the Mac now that flashing
+        # completed successfully so a pairing key isn't sitting on disk.
+        try:
+            if SECRETS_PATH.exists():
+                SECRETS_PATH.unlink()
+        except OSError:
+            # If erase fails for any reason, leave the file — better to let
+            # user recover than to crash the workflow. The file was mode 0600.
+            pass
         return "flashed"
     except subprocess.CalledProcessError:
         return "failed"
@@ -291,8 +300,8 @@ def cmd_run(args) -> int:
 
 
 def _webui_url() -> str | None:
-    """The full tokened link to the helper's page, or None if the helper
-    has never run (that file is written on startup)."""
+    """The full link to the helper's page (no session token in the URL),
+    or None if the helper has never run (that file is written on startup)."""
     try:
         url = webui.URL_PATH.read_text().strip()
     except (OSError, UnicodeDecodeError):
@@ -320,8 +329,8 @@ def cmd_where(args) -> int:
         return 1
     print(url)
     if sys.stderr.isatty():
-        print("\nThat link includes your session key — treat it like a password.\n"
-              "`dashtouch enroll` opens it for you.", file=sys.stderr)
+        print("\nThis link opens the helper's page on localhost; the session token is handled locally and is not included in the URL.\n"
+              "Use `dashtouch enroll` to open it for you.", file=sys.stderr)
     return 0
 
 
@@ -366,7 +375,14 @@ def _daemon_post_setting(key: str, value: int) -> tuple[int, dict]:
     """POST /api/settings against the running helper. Raises on any
     transport failure — callers decide how to report that."""
     base = _daemon_base_url()
-    token = webui.TOKEN_PATH.read_text().strip()
+    # Use the Keychain-stored session token. If Keychain access fails,
+    # the operation cannot proceed securely and we fail with a clear message.
+    try:
+        from . import keychain
+        token = keychain.get_session_token()
+    except Exception as e:
+        raise RuntimeError("Session token unavailable in Keychain. Ensure the helper is running and can access the Keychain.") from e
+
     req = urllib.request.Request(
         base + "api/settings",
         data=json.dumps({"key": key, "value": value}).encode(),

helper/dashtouch_helper/keychain.py

@@ -9,6 +9,23 @@
 
 SERVICE_PASSWORD = "DashboardTouch"
 SERVICE_PAIRING = "DashboardTouch-pairing"
+SERVICE_SESSION = "DashboardTouch-session"
+
+
+def set_session_token(token: str) -> None:
+    """Store the running helper's session token in the macOS Keychain.
+
+    Uses a single account name "session" under SERVICE_SESSION.
+    """
+    _set(SERVICE_SESSION, "session", token)
+
+
+def get_session_token() -> str:
+    """Retrieve the stored session token from the Keychain.
+
+    Raises KeychainError if it couldn't be read.
+    """
+    return _get(SERVICE_SESSION, "session")
 
 
 class KeychainError(Exception):

helper/dashtouch_helper/webui.py

@@ -18,7 +18,10 @@
 WEB_ROOT = pathlib.Path(__file__).parents[1] / "web"
 LABELS_PATH = pathlib.Path.home() / ".dashtouch" / "labels.json"
 URL_PATH = pathlib.Path.home() / ".dashtouch" / "webui-url"
-TOKEN_PATH = pathlib.Path.home() / ".dashtouch" / "token"
+# NOTE: session tokens are stored in the macOS Keychain in production.
+# Tests may still set webui.TOKEN_PATH (see tests/conftest.py) for their
+# fake keychain backing; production code does not read/write TOKEN_PATH.
+TOKEN_PATH = pathlib.Path.home() / ".dashtouch" / "token"  # retained for tests; production uses Keychain only
 
 # The one and only outbound network call this software ever makes, and only
 # when a person clicks the "check for updates" button — see docs/security.md.
@@ -149,6 +152,14 @@ def log_message(self, *a):
         def _json(self, status, obj):
             body = json.dumps(obj).encode()
             self.send_response(status)
+            # Security headers: CSP limits where resources can be loaded from,
+            # X-Frame-Options prevents embedding, Referrer-Policy avoids leaking
+            # the local URL in Referer headers, and nosniff stops MIME sniffing.
+            self.send_header("Content-Security-Policy",
+                             "default-src 'self'; script-src 'self'; style-src 'self'; img-src 'self' data:; object-src 'none'; frame-ancestors 'none';")
+            self.send_header("X-Frame-Options", "DENY")
+            self.send_header("Referrer-Policy", "no-referrer")
+            self.send_header("X-Content-Type-Options", "nosniff")
             self.send_header("Content-Type", "application/json")
             self.send_header("Content-Length", str(len(body)))
             self.end_headers()
@@ -161,6 +172,12 @@ def _static(self, name, ctype):
                 self.send_error(404)
                 return
             self.send_response(200)
+            # Same security headers on static responses.
+            self.send_header("Content-Security-Policy",
+                             "default-src 'self'; script-src 'self'; style-src 'self'; img-src 'self' data:; object-src 'none'; frame-ancestors 'none';")
+            self.send_header("X-Frame-Options", "DENY")
+            self.send_header("Referrer-Policy", "no-referrer")
+            self.send_header("X-Content-Type-Options", "nosniff")
             self.send_header("Content-Type", ctype)
             self.send_header("Content-Length", str(len(body)))
             self.end_headers()
@@ -187,6 +204,10 @@ def do_GET(self):
                 with daemon._events_lock:
                     events = list(daemon.events)
                 self._json(200, {"events": events})
+            elif path == "/api/token":
+                # Same-origin page can fetch the session token; do NOT set CORS
+                # headers so remote origins cannot read this response.
+                self._json(200, {"token": token})
             else:
                 self.send_error(404)
 
@@ -353,17 +374,28 @@ def start(daemon, port: int = 3274) -> str:
             except ValueError:
                 # Invalid env value: ignore, use default
                 pass
-    # Reuse existing token if present and valid; otherwise generate one now
-    # and persist it later, once we know we're not stepping on a live helper.
-    # Token stays valid across restarts by design; delete ~/.dashtouch/token to force fresh one.
+    # Reuse existing token from the macOS Keychain if available; otherwise
+    # generate a fresh one. In production we store the token only in Keychain
+    # (no on-disk token file) to avoid leaking it via the filesystem.
     token = None
+    # Production: always use the macOS Keychain as the persistent
+    # storage for the session token. If Keychain access fails, keep the
+    # token in memory for the running process but do not persist to disk.
     try:
-        existing = TOKEN_PATH.read_text().strip()
-        if len(existing) >= 16:  # plausible token length
+        from . import keychain
+        try:
+            existing = keychain.get_session_token()
+        except Exception:
+            existing = None
+        if existing and len(existing) >= 16:
             token = existing
-    except (OSError, UnicodeDecodeError):
-        # File missing, directory in the way, unreadable, or corrupted — generate fresh token
-        pass
+    except Exception:
+        # Keychain import failed (non-macOS environment) — do not persist to disk.
+        existing = None
+
+    token_freshly_generated = token is None
+    if token is None:
+        token = secrets.token_urlsafe(24)
 
     token_freshly_generated = token is None
     if token is None:
@@ -385,7 +417,13 @@ def start(daemon, port: int = 3274) -> str:
 
     threading.Thread(target=server.serve_forever, daemon=True).start()
     actual = server.server_address[1]
-    url = f"http://127.0.0.1:{actual}/?token={token}"
+    # Publish a token-less URL so the session token is not leaked in the
+    # browser address bar, history, or Referer. The page fetches the token
+    # from /api/token (same-origin) after it loads. For programmatic callers
+    # (and tests) we still return the full token-bearing URL, but the
+    # persisted link on disk intentionally omits the token.
+    serve_url = f"http://127.0.0.1:{actual}/"
+    returned_url = f"http://127.0.0.1:{actual}/?token={token}"
 
     # Don't blindly overwrite the shared link/token files: if they already
     # point at a DIFFERENT port and something is actually answering there,
@@ -414,18 +452,24 @@ def start(daemon, port: int = 3274) -> str:
             print(msg)
 
     if token_freshly_generated and not other_helper_live:
+        # Persist the token to the macOS Keychain. If Keychain write fails,
+        # do not persist to disk — keep the new token in memory only.
         try:
-            TOKEN_PATH.parent.mkdir(parents=True, exist_ok=True)
-            TOKEN_PATH.write_text(token + "\n")
-            TOKEN_PATH.chmod(0o600)
-        except OSError:
-            # Write failed (e.g., parent is a file, permission denied) — proceed with in-memory token
-            # A working daemon with unpersisted token beats a dead one; dashtouch enroll degradation is acceptable
+            from . import keychain
+            try:
+                keychain.set_session_token(token)
+            except Exception:
+                # Can't write to Keychain — intentionally do not persist to disk
+                pass
+        except Exception:
+            # Keychain not available — intentionally keep token in-memory
             pass
 
     if not other_helper_live:
         URL_PATH.parent.mkdir(parents=True, exist_ok=True)
-        URL_PATH.write_text(url + "\n")
+        # Persist the token-less serve URL so the filesystem/stored link can't
+        # leak the session token; return the token-bearing URL to the caller.
+        URL_PATH.write_text(serve_url + "\n")
         URL_PATH.chmod(0o600)
 
-    return url
+    return returned_url

helper/pytest.ini

@@ -0,0 +1,3 @@
+[pytest]
+markers =
+    integration: slow/integration tests that touch network or external services

helper/tests/conftest.py

@@ -4,6 +4,49 @@
 
 @pytest.fixture(autouse=True)
 def isolate_dashtouch_home(tmp_path, monkeypatch):
+    """Isolate filesystem usage for tests and install a fake keychain module
+    that uses the test token file under tmp_path. Tests may still set
+    webui.TOKEN_PATH and the fake keychain will read/write that path, so
+    per-test monkeypatching of TOKEN_PATH continues to work.
+    """
+    import dashtouch_helper.keychain as keychain
+
     monkeypatch.setattr(webui, "URL_PATH", tmp_path / "webui-url")
     monkeypatch.setattr(webui, "LABELS_PATH", tmp_path / "labels.json")
     monkeypatch.setattr(webui, "TOKEN_PATH", tmp_path / "token")
+
+    # Instead of replacing the keychain module in sys.modules, directly
+    # monkeypatch the keychain functions used by the application. This is a
+    # more explicit mock and avoids messing with module resolution.
+    def fake_get_session_token():
+        try:
+            return webui.TOKEN_PATH.read_text().strip()
+        except Exception:
+            # Mirror the KeychainError semantics by raising the module's
+            # KeychainError so production code handles it the same way.
+            raise keychain.KeychainError("no token")
+
+    def fake_set_session_token(token):
+        webui.TOKEN_PATH.parent.mkdir(parents=True, exist_ok=True)
+        webui.TOKEN_PATH.write_text(token + "\n")
+        try:
+            webui.TOKEN_PATH.chmod(0o600)
+        except OSError:
+            pass
+
+    # Wrap the existing _run to convert FileNotFoundError (missing 'security'
+    # binary on non-macOS CI runners) into KeychainError while preserving the
+    # ability for tests to patch subprocess.run and exercise normal flows.
+    orig_run = keychain._run
+
+    def safe_run(args, input_value=None, detach_tty=False):
+        try:
+            return orig_run(args, input_value=input_value, detach_tty=detach_tty)
+        except FileNotFoundError as e:
+            raise keychain.KeychainError("security not available in CI") from e
+
+    monkeypatch.setattr(keychain, "_run", safe_run)
+
+    monkeypatch.setattr(keychain, "get_session_token", fake_get_session_token)
+    monkeypatch.setattr(keychain, "set_session_token", fake_set_session_token)
+