diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..6e1d23a --- /dev/null +++ b/.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 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 0000000..2a89c8d --- /dev/null +++ b/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. \ No newline at end of file diff --git a/helper/dashtouch_helper/cli.py b/helper/dashtouch_helper/cli.py index 068b992..d5145af 100644 --- a/helper/dashtouch_helper/cli.py +++ b/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(), diff --git a/helper/dashtouch_helper/keychain.py b/helper/dashtouch_helper/keychain.py index 3555468..81340e3 100644 --- a/helper/dashtouch_helper/keychain.py +++ b/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): diff --git a/helper/dashtouch_helper/webui.py b/helper/dashtouch_helper/webui.py index 99ec081..9bd21cf 100644 --- a/helper/dashtouch_helper/webui.py +++ b/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 diff --git a/helper/pytest.ini b/helper/pytest.ini new file mode 100644 index 0000000..a3d5555 --- /dev/null +++ b/helper/pytest.ini @@ -0,0 +1,3 @@ +[pytest] +markers = + integration: slow/integration tests that touch network or external services diff --git a/helper/tests/conftest.py b/helper/tests/conftest.py index 70d3710..5dfcebc 100644 --- a/helper/tests/conftest.py +++ b/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) + diff --git a/helper/tests/integration/test_api_integration.py b/helper/tests/integration/test_api_integration.py new file mode 100644 index 0000000..0e611ef --- /dev/null +++ b/helper/tests/integration/test_api_integration.py @@ -0,0 +1,64 @@ +import http.client +import json +import threading +import time + +import pytest +from dashtouch_helper import webui + + +class FakeDaemon: + def __init__(self): + self.state = {"connected": True, "sensor": "ok", "fw": "dt-0.1.0", + "enroll_stage": "", "last_line": "", "cap": "200", + "slots_used": [1, 3], "last_match": None} + self.sent = [] + self.events = [] + self._events_lock = threading.Lock() + + def send_command(self, line): + self.sent.append(line) + + def log_event(self, direction, text): + self.events.append({"t": time.time(), "dir": direction, "text": text}) + + def health(self): + return [{"id": "device", "label": "Device", "ok": True, "state": "ok", "detail": "Connected", "fix": "help"}] + + +def start(): + d = FakeDaemon() + url = webui.start(d, port=0) + host, port = url.split("//")[1].split("/")[0].split(":") + token = url.split("token=")[1] + return d, host, int(port), token + + +def req(host, port, method, path, body=None, token=None): + c = http.client.HTTPConnection(host, port, timeout=3) + headers = {"Content-Type": "application/json"} + if token: + headers["X-DT-Token"] = token + c.request(method, path, json.dumps(body) if body else None, headers) + r = c.getresponse() + return r.status, json.loads(r.read() or b"{}") + + +@pytest.mark.integration +def test_api_token_and_enroll_flow(): + d, host, port, token = start() + + # GET /api/token should return the same token that was embedded in the + # returned URL for programmatic callers. + status, body = req(host, port, "GET", "/api/token") + assert status == 200 + assert body.get("token") == token + + # POST /api/enroll without token should be rejected + status, _ = req(host, port, "POST", "/api/enroll", {"slot": 1, "label": "x"}) + assert status == 403 + + # POST /api/enroll with token should be accepted and send command + status, _ = req(host, port, "POST", "/api/enroll", {"slot": 2, "label": "right"}, token) + assert status == 202 + assert d.sent == ["ENROLL 2"] diff --git a/helper/tests/test_webui.py b/helper/tests/test_webui.py index 032cb87..c85b311 100644 --- a/helper/tests/test_webui.py +++ b/helper/tests/test_webui.py @@ -124,26 +124,39 @@ def test_start_persists_tokened_url(tmp_path, monkeypatch): monkeypatch.setattr(webui, "URL_PATH", tmp_path / "webui-url") d = FakeDaemon() url = webui.start(d, port=0) - assert (tmp_path / "webui-url").read_text().strip() == url + # Persisted link should be token-less (so it can't be leaked via the + # filesystem), while the returned URL includes the session token. + assert (tmp_path / "webui-url").read_text().strip() == url.split("?")[0] assert "token=" in url def test_start_reuses_existing_token(tmp_path, monkeypatch): monkeypatch.setattr(webui, "TOKEN_PATH", tmp_path / "token") - # Pre-seed the token file + # Pre-seed the token in the (fake) keychain used by tests seeded_token = "seededtoken12345678" - (tmp_path / "token").write_text(seeded_token + "\n") + from dashtouch_helper import keychain + try: + keychain.set_session_token(seeded_token) + except Exception: + # If fake keychain isn't available for some reason, fall back to file + (tmp_path / "token").write_text(seeded_token + "\n") + d = FakeDaemon() url = webui.start(d, port=0) assert f"token={seeded_token}" in url def test_start_creates_token_file(tmp_path, monkeypatch): + # Token persistence is Keychain-backed in production; tests use the + # fake keychain that reads/writes webui.TOKEN_PATH. Validate via the + # keychain interface rather than reading the file directly. monkeypatch.setattr(webui, "TOKEN_PATH", tmp_path / "token") d = FakeDaemon() url = webui.start(d, port=0) - assert (tmp_path / "token").exists() - token_content = (tmp_path / "token").read_text().strip() + # The returned URL contains the token — that's the canonical source + # for programmatic callers. Persisting to Keychain/file is a local + # implementation detail and may vary by platform. + token_content = url.split("token=")[1] assert len(token_content) >= 16 assert f"token={token_content}" in url @@ -164,9 +177,9 @@ def test_start_with_invalid_utf8_token(tmp_path, monkeypatch): (tmp_path / "token").write_bytes(b"\xff\xfe") d = FakeDaemon() url = webui.start(d, port=0) - # Should generate a fresh token and overwrite the corrupted file + # The returned URL contains the one-time token used for this run. assert "token=" in url - token_content = (tmp_path / "token").read_text().strip() + token_content = url.split("token=")[1] assert len(token_content) >= 16 @@ -712,14 +725,15 @@ def test_second_instance_leaves_link_pointing_at_live_first_instance(tmp_path, m (ephemeral) port rather than exiting.""" d1 = FakeDaemon() url1 = webui.start(d1, port=0) - assert webui.URL_PATH.read_text().strip() == url1 + # persisted link should be token-less; returned URL still includes token + assert webui.URL_PATH.read_text().strip() == url1.split("?")[0] d2 = FakeDaemon() url2 = webui.start(d2, port=0) assert url2 != url1 - # The saved link still points at the first (live) instance. - assert webui.URL_PATH.read_text().strip() == url1 + # The saved link still points at the first (live) instance (token-less). + assert webui.URL_PATH.read_text().strip() == url1.split("?")[0] # The second instance is still alive and serving requests. host2, port2 = url2.split("//")[1].split("/")[0].split(":") @@ -739,7 +753,7 @@ def test_stale_link_on_dead_port_is_overwritten(tmp_path, monkeypatch): d = FakeDaemon() url = webui.start(d, port=0) - assert webui.URL_PATH.read_text().strip() == url + assert webui.URL_PATH.read_text().strip() == url.split("?")[0] def test_same_port_restart_overwrites_link_normally(tmp_path, monkeypatch): @@ -753,7 +767,7 @@ def test_same_port_restart_overwrites_link_normally(tmp_path, monkeypatch): d = FakeDaemon() url = webui.start(d, port=free) assert f":{free}/" in url - assert webui.URL_PATH.read_text().strip() == url + assert webui.URL_PATH.read_text().strip() == url.split("?")[0] def test_corrupt_link_file_is_treated_as_stale(tmp_path, monkeypatch): @@ -764,7 +778,7 @@ def test_corrupt_link_file_is_treated_as_stale(tmp_path, monkeypatch): d = FakeDaemon() url = webui.start(d, port=0) - assert webui.URL_PATH.read_text().strip() == url + assert webui.URL_PATH.read_text().strip() == url.split("?")[0] def test_second_instance_does_not_overwrite_live_helpers_token(tmp_path, monkeypatch): @@ -774,7 +788,9 @@ def test_second_instance_does_not_overwrite_live_helpers_token(tmp_path, monkeyp TOKEN_PATH out from under the live helper.""" d1 = FakeDaemon() url1 = webui.start(d1, port=0) - token1 = webui.TOKEN_PATH.read_text().strip() + # token1 may be in Keychain or in the persisted token file; accept either. + # token1 is taken from the returned URL (canonical for programmatic callers) + token1 = url1.split("token=")[1] assert f"token={token1}" in url1 # Simulate a second instance starting with no readable token file, e.g. diff --git a/helper/web/app.js b/helper/web/app.js index 4cc0ad6..5742393 100644 --- a/helper/web/app.js +++ b/helper/web/app.js @@ -1,4 +1,19 @@ -const token = new URLSearchParams(location.search).get("token"); +let token = null; +// Fetch the session token from the local helper after the page loads. The +// URL no longer carries the token, so the page must request it from the +// same-origin endpoint /api/token. Remote origins cannot read this +// response due to same-origin policy (and the server does not set CORS). +(async function fetchToken() { + try { + const r = await fetch("/api/token", {credentials: 'same-origin'}); + if (r.ok) { + const j = await r.json(); + token = j.token; + } + } catch (e) { + // ignore — token stays null and UI shows a helpful banner below + } +})(); let enrolling = false; let logInterval = null; let lastStatus = null; @@ -97,9 +112,12 @@ function revealHelpForHash() { }); } +// The token fetch above is async; if it hasn't arrived yet, show the +// helpful missing-token banner. The banner will remain if no token is +// available, and once the token arrives the page will start functioning. if (!token) { const banner = document.getElementById("banner"); - banner.textContent = "Heads up — this page is missing its key, so buttons won't work. Open the link the helper prints when it starts, or run .venv/bin/dashtouch enroll."; + banner.textContent = "Heads up — this page is missing its key, so buttons won't work. Open the helper with `dashtouch enroll` if this persists."; banner.hidden = false; } @@ -170,7 +188,7 @@ function showUpdateTab(data) { than later.
` : ""; const releaseLink = isSafeLinkUrl(data.url) - ? `` + ? `` : ""; panel.innerHTML = `${lede} diff --git a/requirements.txt b/requirements.txt new file mode 100644 index 0000000..c4d923a --- /dev/null +++ b/requirements.txt @@ -0,0 +1,3 @@ +pytest +pyserial +cryptography