From 119c114daa02f1d89734be1c0d9019a528c40245 Mon Sep 17 00:00:00 2001 From: Jason Morcos Date: Sun, 2 Aug 2026 10:02:52 -0700 Subject: [PATCH 1/4] ci: add pull request validation --- .github/workflows/ci.yml | 130 +++++++++++++++++ tests/test_public_api_contract.py | 82 +++++++++++ tests/test_share_safety.py | 96 +++++++++++++ tests/test_worker_cleanup.py | 63 ++++++++ tools/check_distribution.py | 126 ++++++++++++++++ tools/check_share_safety.py | 229 ++++++++++++++++++++++++++++++ 6 files changed, 726 insertions(+) create mode 100644 .github/workflows/ci.yml create mode 100644 tests/test_public_api_contract.py create mode 100644 tests/test_share_safety.py create mode 100644 tests/test_worker_cleanup.py create mode 100755 tools/check_distribution.py create mode 100755 tools/check_share_safety.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..5d8e6a4 --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,130 @@ +name: Validate + +on: + pull_request: + branches: + - main + push: + branches: + - main + workflow_dispatch: + +permissions: + contents: read + +concurrency: + group: validate-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +jobs: + tests: + name: Python ${{ matrix.python-version }} + runs-on: ubuntu-latest + timeout-minutes: 10 + strategy: + fail-fast: false + matrix: + python-version: + - "3.11" + - "3.12" + - "3.13" + - "3.14" + steps: + - uses: actions/checkout@v7 + - uses: actions/setup-python@v7 + with: + python-version: ${{ matrix.python-version }} + - run: python -m pip install --upgrade pip + - run: python -m pip install -e ".[dev]" + - run: python -m pytest -q + + dependency-bounds: + name: Dependencies (${{ matrix.mode }}) + runs-on: ubuntu-latest + timeout-minutes: 10 + strategy: + fail-fast: false + matrix: + include: + - mode: floor + python-version: "3.11" + - mode: latest + python-version: "3.14" + steps: + - uses: actions/checkout@v7 + - uses: actions/setup-python@v7 + with: + python-version: ${{ matrix.python-version }} + - run: python -m pip install --upgrade pip + - if: matrix.mode == 'floor' + run: >- + python -m pip install + "cbor2==5.6.0" + "pyOpenSSL==23.0.0" + "pytest==8.0.0" + - if: matrix.mode == 'floor' + run: python -m pip install --no-deps -e . + - if: matrix.mode == 'latest' + run: python -m pip install -e ".[dev]" + - run: python -m pytest -q + + package: + name: Package artifacts + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@v7 + with: + fetch-depth: 0 + - uses: actions/setup-python@v7 + with: + python-version: "3.14" + - run: python -m pip install --upgrade pip + - run: python -m pip install build hatchling hatch-vcs + - run: python -m build + - run: python tools/check_distribution.py dist + - name: Install and import wheel + run: | + python -m venv "$RUNNER_TEMP/wheel-smoke" + "$RUNNER_TEMP/wheel-smoke/bin/python" -m pip install \ + dist/*.whl + cd "$RUNNER_TEMP" + "$RUNNER_TEMP/wheel-smoke/bin/python" -I -c \ + "from smartthings_local.protocol.dtls_session import DtlsCoapSession" + - name: Install and import sdist + run: | + python -m venv "$RUNNER_TEMP/sdist-smoke" + "$RUNNER_TEMP/sdist-smoke/bin/python" -m pip install \ + dist/*.tar.gz + cd "$RUNNER_TEMP" + "$RUNNER_TEMP/sdist-smoke/bin/python" -I -c \ + "from smartthings_local.ocf.state_cache import StateCache" + + share-safety: + name: Share safety + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - uses: actions/checkout@v7 + with: + fetch-depth: 0 + - uses: actions/setup-python@v7 + with: + python-version: "3.14" + - name: Select comparison base + id: comparison + env: + PR_BASE_SHA: ${{ github.event.pull_request.base.sha }} + PUSH_BEFORE_SHA: ${{ github.event.before }} + run: | + if [ -n "$PR_BASE_SHA" ]; then + echo "sha=$PR_BASE_SHA" >> "$GITHUB_OUTPUT" + elif [ -n "$PUSH_BEFORE_SHA" ] && \ + [ "$PUSH_BEFORE_SHA" != "0000000000000000000000000000000000000000" ]; then + echo "sha=$PUSH_BEFORE_SHA" >> "$GITHUB_OUTPUT" + else + echo "sha=$(git rev-parse HEAD^)" >> "$GITHUB_OUTPUT" + fi + - run: >- + python tools/check_share_safety.py + --changed-since "${{ steps.comparison.outputs.sha }}" diff --git a/tests/test_public_api_contract.py b/tests/test_public_api_contract.py new file mode 100644 index 0000000..0a33e78 --- /dev/null +++ b/tests/test_public_api_contract.py @@ -0,0 +1,82 @@ +"""Compatibility baseline for the published API and LocalThings consumer.""" + +from __future__ import annotations + +import inspect + +from smartthings_local.ocf.observe_refresh import ObserveRefreshTask +from smartthings_local.ocf.state_cache import StateCache +from smartthings_local.protocol.dtls_session import DtlsCoapSession + + +def _parameter_names(callable_object) -> list[str]: + return list(inspect.signature(callable_object).parameters) + + +def test_dtls_session_constructor_keeps_file_memory_and_local_port_inputs(): + assert _parameter_names(DtlsCoapSession) == [ + "host", + "port", + "cert_path", + "key_path", + "cert_pem", + "key_pem", + "on_notification", + "mtu", + "rate_limit_rps", + "local_port", + ] + + +def test_dtls_session_keeps_current_consumer_methods(): + expected = { + "close", + "connect", + "get", + "join", + "pace", + "ping", + "post", + "refresh_observes", + "start_reader", + "subscribe", + } + assert expected <= set(dir(DtlsCoapSession)) + assert _parameter_names(DtlsCoapSession.get) == [ + "self", + "path_segs", + "query", + "timeout", + ] + assert _parameter_names(DtlsCoapSession.post) == [ + "self", + "path_segs", + "body_cbor", + "timeout", + ] + assert _parameter_names(DtlsCoapSession.subscribe) == ["self", "path_segs"] + + +def test_state_cache_keeps_current_consumer_surface(): + assert _parameter_names(StateCache) == ["descriptor"] + expected = { + "apply_optimistic", + "apply_rep", + "freshness_s", + "get", + "index_device_tree", + "set_on_change", + "snapshot", + "stalest", + } + assert expected <= set(dir(StateCache)) + + +def test_observe_refresh_task_keeps_current_consumer_surface(): + assert _parameter_names(ObserveRefreshTask) == [ + "session", + "paths", + "interval_s", + "logger", + ] + assert _parameter_names(ObserveRefreshTask.run_forever) == ["self", "stop"] diff --git a/tests/test_share_safety.py b/tests/test_share_safety.py new file mode 100644 index 0000000..0b971fc --- /dev/null +++ b/tests/test_share_safety.py @@ -0,0 +1,96 @@ +from __future__ import annotations + +import subprocess + +from tools import check_share_safety + + +def test_documentation_addresses_and_synthetic_uuid_are_safe(): + text = ( + "192.0.2.10 198.51.100.20 203.0.113.30 " + "2001:db8::10 11111111-2222-3333-4444-555555555555" + ) + assert check_share_safety.scan_text("fixture.txt", text) == [] + + +def test_findings_never_echo_matched_content(): + cases = { + "PEM_PRIVATE_KEY": "-----BEGIN " + "PRIVATE KEY-----", + "EMAIL_ADDRESS": "person" + "@example.net", + "MAC_ADDRESS": "aa:bb:cc:" + "dd:ee:ff", + "NON_DOCUMENTATION_IPV4": "10." + "24.8.9", + "NON_DOCUMENTATION_IPV6": "fd00" + 2 * chr(58) + "1234", + "PRIVATE_DNS": "appliance" + chr(46) + "house" + chr(46) + "local", + "HOME_PATH": "/" + "Users/person/private.txt", + "CREDENTIAL_URL": "https://user:" + "pass" + chr(64) + "example.net/data", + "SECRET_ASSIGNMENT": ( + "access_token " + chr(61) + " " + chr(34) + "never-print-this" + chr(34) + ), + "SERIAL_ASSIGNMENT": ( + "serialNumber " + chr(61) + " " + chr(34) + "device-123456" + chr(34) + ), + "REAL_TIMESTAMP": "2026-08-02" + "T12:34:56Z", + "QR_PAYLOAD": "qr_" + "payload = value", + "UUID": "12345678-1234-4234-9234-" + "123456789abc", + } + for rule_id, value in cases.items(): + findings = check_share_safety.scan_text("candidate.txt", value) + rendered = "\n".join(finding.render() for finding in findings) + assert f"candidate.txt:1:{rule_id}" in rendered + assert value not in rendered + + +def test_binary_and_archive_inputs_are_rejected(tmp_path): + binary = tmp_path / "fixture.bin" + binary.write_bytes(b"before\x00after") + capture = tmp_path / "fixture.pcap" + capture.write_text("text-looking content") + + assert check_share_safety.scan_file(binary, "fixture.bin") == [ + check_share_safety.Finding("fixture.bin", 0, "BINARY_CONTENT") + ] + assert check_share_safety.scan_file(capture, "fixture.pcap") == [ + check_share_safety.Finding("fixture.pcap", 0, "FORBIDDEN_FILE_TYPE") + ] + + +def test_changed_paths_include_staged_unstaged_and_untracked_files( + tmp_path, monkeypatch +): + def git(*args): + return subprocess.run( + [ + "git", + "-c", + "commit.gpgsign=false", + "-c", + "user.name=Test", + "-c", + "user.email=" + "test" + chr(64) + "example.invalid", + *args, + ], + cwd=tmp_path, + capture_output=True, + check=True, + text=True, + ) + + git("init", "--quiet") + baseline = tmp_path / "baseline.txt" + baseline.write_text("before\n") + git("add", "baseline.txt") + git("commit", "--quiet", "-m", "baseline") + base = git("rev-parse", "HEAD").stdout.strip() + + staged = tmp_path / "staged.txt" + staged.write_text("staged\n") + git("add", "staged.txt") + baseline.write_text("after\n") + (tmp_path / "untracked.txt").write_text("untracked\n") + + monkeypatch.chdir(tmp_path) + assert check_share_safety._changed_paths(base) == [ + "baseline.txt", + "staged.txt", + "untracked.txt", + ] diff --git a/tests/test_worker_cleanup.py b/tests/test_worker_cleanup.py new file mode 100644 index 0000000..080c5e4 --- /dev/null +++ b/tests/test_worker_cleanup.py @@ -0,0 +1,63 @@ +"""Deterministic baseline checks for the current OCF worker stop contract.""" + +from __future__ import annotations + +import threading + +from smartthings_local.ocf.keepalive import KeepaliveTask +from smartthings_local.ocf.observe_refresh import ObserveRefreshTask +from smartthings_local.ocf.poll_scheduler import PollScheduler, PollTier +from smartthings_local.ocf.state_cache import StateCache + + +class _Session: + def ping(self): + return None + + def refresh_observes(self, paths): + return None + + +class _Descriptor: + def on_observation(self, state, href, rep): + return None + + +def _assert_worker_stops(target, name: str): + stop = threading.Event() + entered = threading.Event() + errors: list[str] = [] + + def run(): + entered.set() + try: + target(stop) + except Exception as error: # noqa: BLE001 # pragma: no cover + errors.append(type(error).__name__) + + worker = threading.Thread(target=run, name=name) + worker.start() + assert entered.wait(0.5), f"{name} did not enter its worker" + stop.set() + worker.join(0.5) + assert not worker.is_alive(), f"{name} did not stop" + assert errors == [], f"{name} raised {errors[0]}" + + +def test_keepalive_worker_stops_without_waiting_for_interval(): + task = KeepaliveTask(_Session(), interval_s=3600.0) + _assert_worker_stops(task.run_forever, "test-keepalive") + + +def test_observe_refresh_worker_stops_without_waiting_for_interval(): + task = ObserveRefreshTask(_Session(), [], interval_s=3600.0) + _assert_worker_stops(task.run_forever, "test-observe-refresh") + + +def test_poll_scheduler_worker_stops_without_leaking_thread(): + scheduler = PollScheduler( + _Session(), + StateCache(_Descriptor()), + [PollTier("idle", interval_s=3600.0, paths=())], + ) + _assert_worker_stops(scheduler.run_forever, "test-poll-scheduler") diff --git a/tools/check_distribution.py b/tools/check_distribution.py new file mode 100755 index 0000000..c246b92 --- /dev/null +++ b/tools/check_distribution.py @@ -0,0 +1,126 @@ +#!/usr/bin/env python3 +"""Verify that SmartThings-Local wheel and sdist contents are intentional.""" + +from __future__ import annotations + +import argparse +import subprocess +import tarfile +import zipfile +from pathlib import Path, PurePosixPath + + +class DistributionError(RuntimeError): + """An artifact contains a missing, unexpected, or unsafe member.""" + + +def _tracked_files() -> set[str]: + proc = subprocess.run( + ["git", "ls-files", "-z", "--", "smartthings_local", "tests"], + capture_output=True, + check=True, + ) + return {value.decode("utf-8") for value in proc.stdout.split(b"\0") if value} + + +def _safe_member(name: str) -> bool: + path = PurePosixPath(name) + return bool(name) and not path.is_absolute() and ".." not in path.parts + + +def _expected_package_files() -> set[str]: + tracked = { + path for path in _tracked_files() if path.startswith("smartthings_local/") + } + tracked.add("smartthings_local/_version.py") + return tracked + + +def check_wheel(path: Path) -> None: + with zipfile.ZipFile(path) as archive: + names = set(archive.namelist()) + if not names or any(not _safe_member(name) for name in names): + raise DistributionError("wheel has an unsafe member") + + package_files = {name for name in names if name.startswith("smartthings_local/")} + if package_files != _expected_package_files(): + raise DistributionError( + "wheel package contents differ from the tracked package" + ) + + metadata = names - package_files + roots = {name.split("/", 1)[0] for name in metadata} + if len(roots) != 1: + raise DistributionError("wheel must contain one dist-info directory") + dist_info = roots.pop() + if not dist_info.endswith(".dist-info"): + raise DistributionError("wheel metadata directory is invalid") + expected_metadata = { + f"{dist_info}/METADATA", + f"{dist_info}/WHEEL", + f"{dist_info}/licenses/LICENSE", + f"{dist_info}/RECORD", + } + if metadata != expected_metadata: + raise DistributionError("wheel metadata contents are unexpected") + + +def check_sdist(path: Path) -> None: + with tarfile.open(path, mode="r:gz") as archive: + members = archive.getmembers() + if not members or any( + not member.isfile() or member.issym() or member.islnk() for member in members + ): + raise DistributionError("sdist must contain regular files only") + names = {member.name for member in members} + if any(not _safe_member(name) for name in names): + raise DistributionError("sdist has an unsafe member") + + roots = {name.split("/", 1)[0] for name in names} + if len(roots) != 1: + raise DistributionError("sdist must contain one top-level directory") + root = roots.pop() + relative = {name[len(root) + 1 :] for name in names if name.startswith(f"{root}/")} + expected = _tracked_files() | { + ".gitignore", + ".hgignore", + "LICENSE", + "PKG-INFO", + "README.md", + "pyproject.toml", + "smartthings_local/_version.py", + } + if relative != expected: + raise DistributionError("sdist contents differ from the intended source set") + + +def check_directory(directory: Path) -> None: + wheels = sorted(directory.glob("*.whl")) + sdists = sorted(directory.glob("*.tar.gz")) + if len(wheels) != 1 or len(sdists) != 1: + raise DistributionError("expected exactly one wheel and one sdist") + check_wheel(wheels[0]) + check_sdist(sdists[0]) + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("directory", type=Path) + args = parser.parse_args() + try: + check_directory(args.directory) + except ( + DistributionError, + OSError, + subprocess.SubprocessError, + tarfile.TarError, + zipfile.BadZipFile, + ): + print("distribution check failed") + return 1 + print("distribution contents verified") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tools/check_share_safety.py b/tools/check_share_safety.py new file mode 100755 index 0000000..b35907d --- /dev/null +++ b/tools/check_share_safety.py @@ -0,0 +1,229 @@ +#!/usr/bin/env python3 +"""Check changed public files for common private-data and secret shapes. + +Findings contain only path, line, and rule ID. Matched content is never +printed because it may itself be sensitive. +""" + +from __future__ import annotations + +import argparse +import ipaddress +import re +import subprocess +from collections.abc import Iterable +from dataclasses import dataclass +from pathlib import Path + +MAX_TEXT_BYTES = 2 * 1024 * 1024 +DOCUMENTATION_IPV4 = tuple( + ipaddress.ip_network(value) + for value in ("192.0.2.0/24", "198.51.100.0/24", "203.0.113.0/24") +) +DOCUMENTATION_IPV6 = ipaddress.ip_network("2001:db8::/32") +SAFE_UUIDS = { + "00000000-0000-0000-0000-000000000000", + "11111111-2222-3333-4444-555555555555", +} +FORBIDDEN_SUFFIXES = { + ".7z", + ".apk", + ".cap", + ".db", + ".der", + ".gz", + ".jks", + ".key", + ".p12", + ".pcap", + ".pcapng", + ".pfx", + ".sqlite", + ".sqlite3", + ".tar", + ".tgz", + ".zip", +} + +PATTERNS = ( + ( + "PEM_PRIVATE_KEY", + re.compile(r"-----BEGIN (?:RSA |EC |OPENSSH |DSA )?PRIVATE KEY-----"), + ), + ( + "EMAIL_ADDRESS", + re.compile(r"[A-Za-z0-9._%+-]+@(?:[A-Za-z0-9-]+\.)+[A-Za-z]{2,}"), + ), + ( + "MAC_ADDRESS", + re.compile(r"(?i)(? str: + return f"{self.path}:{self.line}:{self.rule_id}" + + +def _safe_ipv4(value: str) -> bool: + address = ipaddress.ip_address(value) + return ( + address.is_loopback + or address.is_unspecified + or any(address in network for network in DOCUMENTATION_IPV4) + ) + + +def _safe_ipv6(value: str) -> bool: + address = ipaddress.ip_address(value.strip("[]").split("%", 1)[0]) + return ( + address.is_loopback or address.is_unspecified or address in DOCUMENTATION_IPV6 + ) + + +def scan_text(path: str, text: str) -> list[Finding]: + findings: set[Finding] = set() + for line_number, line in enumerate(text.splitlines(), start=1): + for rule_id, pattern in PATTERNS: + if pattern.search(line): + findings.add(Finding(path, line_number, rule_id)) + for match in UUID_RE.finditer(line): + if match.group(0).lower() not in SAFE_UUIDS: + findings.add(Finding(path, line_number, "UUID")) + for match in IPV4_RE.finditer(line): + if not _safe_ipv4(match.group(0)): + findings.add(Finding(path, line_number, "NON_DOCUMENTATION_IPV4")) + for match in IPV6_RE.finditer(line): + try: + safe = _safe_ipv6(match.group(0)) + except ValueError: + continue + if not safe: + findings.add(Finding(path, line_number, "NON_DOCUMENTATION_IPV6")) + return sorted(findings) + + +def scan_file(path: Path, display_path: str) -> list[Finding]: + if path.is_symlink(): + return [Finding(display_path, 0, "SYMLINK")] + if path.suffix.casefold() in FORBIDDEN_SUFFIXES: + return [Finding(display_path, 0, "FORBIDDEN_FILE_TYPE")] + data = path.read_bytes() + if len(data) > MAX_TEXT_BYTES: + return [Finding(display_path, 0, "FILE_TOO_LARGE")] + if b"\x00" in data: + return [Finding(display_path, 0, "BINARY_CONTENT")] + try: + text = data.decode("utf-8", errors="strict") + except UnicodeDecodeError: + return [Finding(display_path, 0, "NON_UTF8_CONTENT")] + return scan_text(display_path, text) + + +def _changed_paths(base: str) -> list[str]: + commands = ( + [ + "git", + "diff", + "--name-only", + "--diff-filter=ACMR", + "-z", + f"{base}..HEAD", + "--", + ], + ["git", "diff", "--cached", "--name-only", "--diff-filter=ACMR", "-z", "--"], + ["git", "diff", "--name-only", "--diff-filter=ACMR", "-z", "--"], + ) + changed = [ + subprocess.run(command, capture_output=True, check=True) for command in commands + ] + untracked = subprocess.run( + ["git", "ls-files", "--others", "--exclude-standard", "-z"], + capture_output=True, + check=True, + ) + return sorted( + { + value.decode("utf-8") + for output in (*(result.stdout for result in changed), untracked.stdout) + for value in output.split(b"\0") + if value + } + ) + + +def check_paths(paths: Iterable[str]) -> list[Finding]: + findings: list[Finding] = [] + for value in paths: + findings.extend(scan_file(Path(value), value)) + return sorted(set(findings)) + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("paths", nargs="*") + parser.add_argument("--changed-since") + args = parser.parse_args() + try: + paths = _changed_paths(args.changed_since) if args.changed_since else args.paths + if not paths: + raise ValueError("no paths selected") + findings = check_paths(paths) + except (OSError, UnicodeDecodeError, subprocess.SubprocessError, ValueError): + print("share-safety scan failed closed:SCAN_ERROR") + return 2 + for finding in findings: + print(finding.render()) + return 1 if findings else 0 + + +if __name__ == "__main__": + raise SystemExit(main()) From 23338995bfcd31759cf0a5b8dc45f7cbbba0c01c Mon Sep 17 00:00:00 2001 From: Jason Morcos Date: Sun, 2 Aug 2026 10:07:49 -0700 Subject: [PATCH 2/4] build: require pyOpenSSL DTLS timeout support --- .github/workflows/ci.yml | 2 +- pyproject.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5d8e6a4..5dea2da 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -60,7 +60,7 @@ jobs: run: >- python -m pip install "cbor2==5.6.0" - "pyOpenSSL==23.0.0" + "pyOpenSSL==23.1.0" "pytest==8.0.0" - if: matrix.mode == 'floor' run: python -m pip install --no-deps -e . diff --git a/pyproject.toml b/pyproject.toml index 4ae1273..0706be1 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -21,7 +21,7 @@ classifiers = [ ] dependencies = [ "cbor2>=5.6", - "pyOpenSSL>=23.0", + "pyOpenSSL>=23.1", ] [project.urls] From 2c93cb3097cd21e054345c213febc3845e491c31 Mon Sep 17 00:00:00 2001 From: Jason Morcos Date: Sun, 2 Aug 2026 10:13:56 -0700 Subject: [PATCH 3/4] test: make worker cleanup checks deterministic --- tests/test_worker_cleanup.py | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/tests/test_worker_cleanup.py b/tests/test_worker_cleanup.py index 080c5e4..995fd59 100644 --- a/tests/test_worker_cleanup.py +++ b/tests/test_worker_cleanup.py @@ -23,21 +23,29 @@ class _Descriptor: return None +class _ObservedEvent(threading.Event): + def __init__(self): + super().__init__() + self.waiting = threading.Event() + + def wait(self, timeout=None): + self.waiting.set() + return super().wait(timeout) + + def _assert_worker_stops(target, name: str): - stop = threading.Event() - entered = threading.Event() + stop = _ObservedEvent() errors: list[str] = [] def run(): - entered.set() try: target(stop) except Exception as error: # noqa: BLE001 # pragma: no cover errors.append(type(error).__name__) - worker = threading.Thread(target=run, name=name) + worker = threading.Thread(target=run, name=name, daemon=True) worker.start() - assert entered.wait(0.5), f"{name} did not enter its worker" + assert stop.waiting.wait(0.5), f"{name} did not enter an interruptible wait" stop.set() worker.join(0.5) assert not worker.is_alive(), f"{name} did not stop" From fc6240b72e5da90fed732ed26a2ebe1e20770406 Mon Sep 17 00:00:00 2001 From: Jason Morcos Date: Sun, 2 Aug 2026 15:33:45 -0700 Subject: [PATCH 4/4] test(safety): allow public GitHub attachments --- tests/test_public_api_contract.py | 99 ++++++++++++++++++++----------- tests/test_share_safety.py | 62 +++++++++++++++++++ tests/test_worker_cleanup.py | 8 ++- tools/check_share_safety.py | 91 ++++++++++++++++++++++++++-- 4 files changed, 217 insertions(+), 43 deletions(-) diff --git a/tests/test_public_api_contract.py b/tests/test_public_api_contract.py index 0a33e78..8467f6a 100644 --- a/tests/test_public_api_contract.py +++ b/tests/test_public_api_contract.py @@ -9,23 +9,37 @@ from smartthings_local.ocf.state_cache import StateCache from smartthings_local.protocol.dtls_session import DtlsCoapSession -def _parameter_names(callable_object) -> list[str]: - return list(inspect.signature(callable_object).parameters) +def _assert_compatible_signature(callable_object, expected: list[str]) -> None: + """Require the existing call surface while allowing safe extensions.""" + parameters = list(inspect.signature(callable_object).parameters.values()) + assert [parameter.name for parameter in parameters[: len(expected)]] == expected + for parameter in parameters[len(expected) :]: + assert ( + parameter.kind + in ( + inspect.Parameter.VAR_POSITIONAL, + inspect.Parameter.VAR_KEYWORD, + ) + or parameter.default is not inspect.Parameter.empty + ) def test_dtls_session_constructor_keeps_file_memory_and_local_port_inputs(): - assert _parameter_names(DtlsCoapSession) == [ - "host", - "port", - "cert_path", - "key_path", - "cert_pem", - "key_pem", - "on_notification", - "mtu", - "rate_limit_rps", - "local_port", - ] + _assert_compatible_signature( + DtlsCoapSession, + [ + "host", + "port", + "cert_path", + "key_path", + "cert_pem", + "key_pem", + "on_notification", + "mtu", + "rate_limit_rps", + "local_port", + ], + ) def test_dtls_session_keeps_current_consumer_methods(): @@ -42,23 +56,32 @@ def test_dtls_session_keeps_current_consumer_methods(): "subscribe", } assert expected <= set(dir(DtlsCoapSession)) - assert _parameter_names(DtlsCoapSession.get) == [ - "self", - "path_segs", - "query", - "timeout", - ] - assert _parameter_names(DtlsCoapSession.post) == [ - "self", - "path_segs", - "body_cbor", - "timeout", - ] - assert _parameter_names(DtlsCoapSession.subscribe) == ["self", "path_segs"] + _assert_compatible_signature( + DtlsCoapSession.get, + [ + "self", + "path_segs", + "query", + "timeout", + ], + ) + _assert_compatible_signature( + DtlsCoapSession.post, + [ + "self", + "path_segs", + "body_cbor", + "timeout", + ], + ) + _assert_compatible_signature( + DtlsCoapSession.subscribe, + ["self", "path_segs"], + ) def test_state_cache_keeps_current_consumer_surface(): - assert _parameter_names(StateCache) == ["descriptor"] + _assert_compatible_signature(StateCache, ["descriptor"]) expected = { "apply_optimistic", "apply_rep", @@ -73,10 +96,16 @@ def test_state_cache_keeps_current_consumer_surface(): def test_observe_refresh_task_keeps_current_consumer_surface(): - assert _parameter_names(ObserveRefreshTask) == [ - "session", - "paths", - "interval_s", - "logger", - ] - assert _parameter_names(ObserveRefreshTask.run_forever) == ["self", "stop"] + _assert_compatible_signature( + ObserveRefreshTask, + [ + "session", + "paths", + "interval_s", + "logger", + ], + ) + _assert_compatible_signature( + ObserveRefreshTask.run_forever, + ["self", "stop"], + ) diff --git a/tests/test_share_safety.py b/tests/test_share_safety.py index 0b971fc..d948b3d 100644 --- a/tests/test_share_safety.py +++ b/tests/test_share_safety.py @@ -13,6 +13,22 @@ def test_documentation_addresses_and_synthetic_uuid_are_safe(): assert check_share_safety.scan_text("fixture.txt", text) == [] +def test_dotted_object_identifiers_are_not_ipv4_addresses(): + text = "extendedKeyUsage = 1.3.6.1.4.1.51414.0.1.2" + + assert check_share_safety.scan_text("fixture.txt", text) == [] + + +def test_public_github_attachment_uuid_is_safe_but_bare_uuid_is_not(): + value = "cc1dca15-f272-4625-" + "a13c-2dc82283ff95" + public_url = f"https://github.com/user-attachments/assets/{value}" + + assert check_share_safety.scan_text("README.md", public_url) == [] + assert check_share_safety.scan_text("fixture.txt", value) == [ + check_share_safety.Finding("fixture.txt", 1, "UUID") + ] + + def test_findings_never_echo_matched_content(): cases = { "PEM_PRIVATE_KEY": "-----BEGIN " + "PRIVATE KEY-----", @@ -94,3 +110,49 @@ def test_changed_paths_include_staged_unstaged_and_untracked_files( "staged.txt", "untracked.txt", ] + + +def test_committed_scan_ignores_unchanged_findings_but_checks_added_lines( + tmp_path, monkeypatch +): + def git(*args): + return subprocess.run( + [ + "git", + "-c", + "commit.gpgsign=false", + "-c", + "user.name=Test", + "-c", + "user.email=" + "test" + chr(64) + "example.invalid", + *args, + ], + cwd=tmp_path, + capture_output=True, + check=True, + text=True, + ) + + candidate = tmp_path / "candidate.txt" + private_one = "10." + "24.8.9" + private_two = "10." + "24.8.10" + git("init", "--quiet") + candidate.write_text(f"existing {private_one}\n") + git("add", "candidate.txt") + git("commit", "--quiet", "-m", "baseline") + base = git("rev-parse", "HEAD").stdout.strip() + + candidate.write_text(f"existing {private_one}\nsafe addition\n") + git("add", "candidate.txt") + git("commit", "--quiet", "-m", "safe change") + monkeypatch.chdir(tmp_path) + assert check_share_safety.check_changed(base) == [] + + candidate.write_text( + f"existing {private_one}\nsafe addition\nintroduced {private_two}\n" + ) + git("add", "candidate.txt") + git("commit", "--quiet", "-m", "unsafe change") + assert check_share_safety.check_changed(base) == [ + check_share_safety.Finding("candidate.txt", 3, "NON_DOCUMENTATION_IPV4") + ] diff --git a/tests/test_worker_cleanup.py b/tests/test_worker_cleanup.py index 995fd59..11694b6 100644 --- a/tests/test_worker_cleanup.py +++ b/tests/test_worker_cleanup.py @@ -9,6 +9,8 @@ from smartthings_local.ocf.observe_refresh import ObserveRefreshTask from smartthings_local.ocf.poll_scheduler import PollScheduler, PollTier from smartthings_local.ocf.state_cache import StateCache +_THREAD_DEADLINE_S = 2.0 + class _Session: def ping(self): @@ -45,9 +47,11 @@ def _assert_worker_stops(target, name: str): worker = threading.Thread(target=run, name=name, daemon=True) worker.start() - assert stop.waiting.wait(0.5), f"{name} did not enter an interruptible wait" + assert stop.waiting.wait(_THREAD_DEADLINE_S), ( + f"{name} did not enter an interruptible wait" + ) stop.set() - worker.join(0.5) + worker.join(_THREAD_DEADLINE_S) assert not worker.is_alive(), f"{name} did not stop" assert errors == [], f"{name} raised {errors[0]}" diff --git a/tools/check_share_safety.py b/tools/check_share_safety.py index b35907d..24ca068 100755 --- a/tools/check_share_safety.py +++ b/tools/check_share_safety.py @@ -1,5 +1,5 @@ #!/usr/bin/env python3 -"""Check changed public files for common private-data and secret shapes. +"""Check introduced public content for common private-data and secret shapes. Findings contain only path, line, and rule ID. Matched content is never printed because it may itself be sensitive. @@ -92,11 +92,14 @@ PATTERNS = ( re.compile(r"(?i)\b(?:qr[_-]?payload|setup[_-]?payload)\b\s*(?::|=)"), ), ) -UUID_RE = re.compile( - r"(?i)(?{UUID_PATTERN})", + re.IGNORECASE, ) IPV4_RE = re.compile( - r"(? bool: def scan_text(path: str, text: str) -> list[Finding]: findings: set[Finding] = set() for line_number, line in enumerate(text.splitlines(), start=1): + public_attachment_uuids = { + match.group("uuid").lower() + for match in PUBLIC_GITHUB_ATTACHMENT_RE.finditer(line) + } for rule_id, pattern in PATTERNS: if pattern.search(line): findings.add(Finding(path, line_number, rule_id)) for match in UUID_RE.finditer(line): - if match.group(0).lower() not in SAFE_UUIDS: + value = match.group(0).lower() + if value not in SAFE_UUIDS and value not in public_attachment_uuids: findings.add(Finding(path, line_number, "UUID")) for match in IPV4_RE.finditer(line): if not _safe_ipv4(match.group(0)): @@ -200,6 +208,73 @@ def _changed_paths(base: str) -> list[str]: ) +def _local_changed_paths() -> set[str]: + commands = ( + ["git", "diff", "--cached", "--name-only", "--diff-filter=ACMR", "-z", "--"], + ["git", "diff", "--name-only", "--diff-filter=ACMR", "-z", "--"], + ["git", "ls-files", "--others", "--exclude-standard", "-z"], + ) + outputs = ( + subprocess.run(command, capture_output=True, check=True).stdout + for command in commands + ) + return { + value.decode("utf-8") + for output in outputs + for value in output.split(b"\0") + if value + } + + +HUNK_RE = re.compile(r"^@@ -\d+(?:,\d+)? \+(?P\d+)(?:,(?P\d+))? @@") + + +def _introduced_lines(base: str, path: str) -> set[int]: + result = subprocess.run( + [ + "git", + "diff", + "--no-color", + "--no-ext-diff", + "--unified=0", + "--diff-filter=ACMR", + f"{base}..HEAD", + "--", + path, + ], + capture_output=True, + check=True, + text=True, + ) + lines: set[int] = set() + for value in result.stdout.splitlines(): + match = HUNK_RE.match(value) + if match is None: + continue + start = int(match.group("start")) + count = int(match.group("count") or 1) + lines.update(range(start, start + count)) + return lines + + +def check_changed(base: str) -> list[Finding]: + """Scan introduced committed lines and all local-only file content.""" + local_paths = _local_changed_paths() + findings: list[Finding] = [] + for path in _changed_paths(base): + path_findings = scan_file(Path(path), path) + if path in local_paths: + findings.extend(path_findings) + continue + introduced = _introduced_lines(base, path) + findings.extend( + finding + for finding in path_findings + if finding.line == 0 or finding.line in introduced + ) + return sorted(set(findings)) + + def check_paths(paths: Iterable[str]) -> list[Finding]: findings: list[Finding] = [] for value in paths: @@ -216,7 +291,11 @@ def main() -> int: paths = _changed_paths(args.changed_since) if args.changed_since else args.paths if not paths: raise ValueError("no paths selected") - findings = check_paths(paths) + findings = ( + check_changed(args.changed_since) + if args.changed_since + else check_paths(paths) + ) except (OSError, UnicodeDecodeError, subprocess.SubprocessError, ValueError): print("share-safety scan failed closed:SCAN_ERROR") return 2