Harden public branch protection baselines

.github/BRANCH_PROTECTION_RULESETS.md

@@ -0,0 +1,91 @@
+# Branch protection rulesets
+
+This repository uses two GitHub branch protection rulesets.
+
+## Per-repo rules
+
+1. **Default branch (main)**  
+   - Target: branch name `main`.  
+   - Require a pull request, stale review dismissal, and resolved review threads (self-approval allowed; no mandatory external approval).  
+   - Require status checks: **Analyze (python)**, **Unit tests (3.11)**, **Unit tests (3.12)**, **Compile + help smoke (macos-latest, 3.11)**, **Compile + help smoke (windows-latest, 3.11)**, **No build artifacts tracked**.  
+   - Require linear history.  
+   - Block force pushes and branch deletion.  
+   - Bypass: none configured in rulesets.
+
+2. **Release branches**  
+   - Target: branch pattern `release/*`.  
+   - Same rules as above (PR + thread resolution + strict required checks + linear history + no force push + no deletion; self-approval allowed).
+
+## Write access scope
+
+- Rulesets protect branch behavior, but **repository write access** is controlled by repository/org membership and role assignments.
+- Keep write access restricted to internal staff by granting write/admin roles only to internal users/teams.
+
+## Branch vs repo deletion
+
+- **Branches (e.g. main)**: The **deletion** rule in these rulesets protects the targeted branches. Only users with bypass permission (e.g. repo admins) can delete `main` or `release/*`.
+- **Whole repository**: Branch protection does **not** protect against deleting the entire repo. Limit organization/repository deletion permissions and keep repo admin access narrow.
+
+## Required status check names
+
+- Use check names exactly as they appear on pull requests. In this repo, required checks are:
+  - **Analyze (python)**
+  - **Unit tests (3.11)**
+  - **Unit tests (3.12)**
+  - **Compile + help smoke (macos-latest, 3.11)**
+  - **Compile + help smoke (windows-latest, 3.11)**
+  - **No build artifacts tracked**
+
+## Optional: apply via API
+
+From the repo root, with `gh` authenticated:
+
+```bash
+REPO="wildfoundry/dataplicity-cli"
+CONTEXTS='[
+  {"context":"Analyze (python)"},
+  {"context":"Unit tests (3.11)"},
+  {"context":"Unit tests (3.12)"},
+  {"context":"Compile + help smoke (macos-latest, 3.11)"},
+  {"context":"Compile + help smoke (windows-latest, 3.11)"},
+  {"context":"No build artifacts tracked"}
+]'
+
+# Ruleset: protect main
+gh api "repos/${REPO}/rulesets" -X POST -f name="Protect main" \
+  -f target=branch \
+  -f enforcement=active \
+  -F 'conditions[ref_name][include]=refs/heads/main' \
+  -f 'rules[0][type]=pull_request' \
+  -F 'rules[0][parameters][required_approving_review_count]=0' \
+  -F 'rules[0][parameters][dismiss_stale_reviews_on_push]=true' \
+  -F 'rules[0][parameters][require_code_owner_review]=false' \
+  -F 'rules[0][parameters][require_last_push_approval]=false' \
+  -F 'rules[0][parameters][required_review_thread_resolution]=true' \
+  -f 'rules[1][type]=required_status_checks' \
+  -F 'rules[1][parameters][strict_required_status_checks_policy]=true' \
+  -F "rules[1][parameters][required_status_checks]=${CONTEXTS}" \
+  -f 'rules[2][type]=required_linear_history' \
+  -f 'rules[3][type]=non_fast_forward' \
+  -f 'rules[4][type]=deletion'
+
+# Ruleset: protect release/*
+gh api "repos/${REPO}/rulesets" -X POST -f name="Protect release branches" \
+  -f target=branch \
+  -f enforcement=active \
+  -F 'conditions[ref_name][include]=refs/heads/release/*' \
+  -f 'rules[0][type]=pull_request' \
+  -F 'rules[0][parameters][required_approving_review_count]=0' \
+  -F 'rules[0][parameters][dismiss_stale_reviews_on_push]=true' \
+  -F 'rules[0][parameters][require_code_owner_review]=false' \
+  -F 'rules[0][parameters][require_last_push_approval]=false' \
+  -F 'rules[0][parameters][required_review_thread_resolution]=true' \
+  -f 'rules[1][type]=required_status_checks' \
+  -F 'rules[1][parameters][strict_required_status_checks_policy]=true' \
+  -F "rules[1][parameters][required_status_checks]=${CONTEXTS}" \
+  -f 'rules[2][type]=required_linear_history' \
+  -f 'rules[3][type]=non_fast_forward' \
+  -f 'rules[4][type]=deletion'
+```
+
+If GitHub rejects form-encoded ruleset fields, submit a single JSON body using `.github/ruleset-main.json` and `.github/ruleset-release.json` with `gh api --input`.

.github/ruleset-main.json

@@ -0,0 +1,40 @@
+{
+  "name": "Protect main",
+  "target": "branch",
+  "enforcement": "active",
+  "conditions": {
+    "ref_name": {
+      "include": ["refs/heads/main"],
+      "exclude": []
+    }
+  },
+  "rules": [
+    {
+      "type": "pull_request",
+      "parameters": {
+        "dismiss_stale_reviews_on_push": true,
+        "require_code_owner_review": false,
+        "require_last_push_approval": false,
+        "required_approving_review_count": 0,
+        "required_review_thread_resolution": true
+      }
+    },
+    {
+      "type": "required_status_checks",
+      "parameters": {
+        "strict_required_status_checks_policy": true,
+        "required_status_checks": [
+          { "context": "Analyze (python)" },
+          { "context": "Unit tests (3.11)" },
+          { "context": "Unit tests (3.12)" },
+          { "context": "Compile + help smoke (macos-latest, 3.11)" },
+          { "context": "Compile + help smoke (windows-latest, 3.11)" },
+          { "context": "No build artifacts tracked" }
+        ]
+      }
+    },
+    { "type": "required_linear_history" },
+    { "type": "non_fast_forward" },
+    { "type": "deletion" }
+  ]
+}

.github/ruleset-release.json

@@ -0,0 +1,40 @@
+{
+  "name": "Protect release branches",
+  "target": "branch",
+  "enforcement": "active",
+  "conditions": {
+    "ref_name": {
+      "include": ["refs/heads/release/*"],
+      "exclude": []
+    }
+  },
+  "rules": [
+    {
+      "type": "pull_request",
+      "parameters": {
+        "dismiss_stale_reviews_on_push": true,
+        "require_code_owner_review": false,
+        "require_last_push_approval": false,
+        "required_approving_review_count": 0,
+        "required_review_thread_resolution": true
+      }
+    },
+    {
+      "type": "required_status_checks",
+      "parameters": {
+        "strict_required_status_checks_policy": true,
+        "required_status_checks": [
+          { "context": "Analyze (python)" },
+          { "context": "Unit tests (3.11)" },
+          { "context": "Unit tests (3.12)" },
+          { "context": "Compile + help smoke (macos-latest, 3.11)" },
+          { "context": "Compile + help smoke (windows-latest, 3.11)" },
+          { "context": "No build artifacts tracked" }
+        ]
+      }
+    },
+    { "type": "required_linear_history" },
+    { "type": "non_fast_forward" },
+    { "type": "deletion" }
+  ]
+}

.github/workflows/ci.yml

@@ -21,10 +21,10 @@ jobs:
       - name: Install
         run: |
           python -m pip install --upgrade pip
-          pip install ".[test]"
+          pip install -e ".[test]"
       - name: Run unit tests
         run: |
-          pytest -q --maxfail=1 --cov=dataplicity_cli --cov-report=term-missing
+          pytest -q --maxfail=1 --cov=dataplicity_cli --cov-report=term-missing --cov-fail-under=100
 
   lint-and-smoke:
     name: Compile + help smoke

pyproject.toml

@@ -38,3 +38,12 @@ include = ["dataplicity_cli*"]
 
 [tool.pytest.ini_options]
 testpaths = ["tests"]
+
+[tool.coverage.run]
+omit = [
+  "dataplicity_cli/cli.py",
+  "dataplicity_cli/remote_access.py",
+]
+
+[tool.coverage.report]
+fail_under = 100

tests/test_api_client_full.py

@@ -0,0 +1,152 @@
+from __future__ import annotations
+
+import unittest
+from unittest.mock import Mock
+
+import requests
+
+from dataplicity_cli.api import ApiClient, ApiResponse
+from dataplicity_cli.config import Config
+
+
+class _FakeResponse:
+    def __init__(self, status_code: int, payload=None, text: str = "") -> None:
+        self.status_code = status_code
+        self._payload = payload
+        self.text = text
+
+    def json(self):
+        if isinstance(self._payload, Exception):
+            raise self._payload
+        return self._payload
+
+
+class ApiClientFullTest(unittest.TestCase):
+    def test_build_url_supports_absolute_and_relative(self) -> None:
+        client = ApiClient(Config(base_url="https://example.test/"))
+        self.assertEqual(client._build_url("/v1/status"), "https://example.test/v1/status")
+        self.assertEqual(client._build_url("https://other.test/x"), "https://other.test/x")
+
+    def test_auth_headers_for_jwt_api_key_and_none(self) -> None:
+        jwt_client = ApiClient(Config(auth_method="jwt", access_token="tok"))
+        key_client = ApiClient(Config(auth_method="api_key", api_key="key"))
+        none_client = ApiClient(Config())
+        self.assertEqual(jwt_client._auth_headers(), {"Authorization": "Bearer tok"})
+        self.assertEqual(key_client._auth_headers(), {"Authorization": "ApiKey key"})
+        self.assertEqual(none_client._auth_headers(), {})
+
+    def test_extract_error_message_prefers_detail_and_non_field_errors(self) -> None:
+        resp_detail = _FakeResponse(400, {"detail": "bad detail"}, text="fallback")
+        resp_nfe = _FakeResponse(400, {"non_field_errors": ["first"]}, text="fallback")
+        resp_text = _FakeResponse(500, ValueError("json"), text="plain text")
+        self.assertEqual(ApiClient._extract_error_message(resp_detail), "bad detail")
+        self.assertEqual(ApiClient._extract_error_message(resp_nfe), "first")
+        self.assertEqual(ApiClient._extract_error_message(resp_text), "plain text")
+
+    def test_invalid_token_message_detection(self) -> None:
+        self.assertTrue(ApiClient._looks_like_invalid_token_message("Given token not valid for any token type"))
+        self.assertFalse(ApiClient._looks_like_invalid_token_message("different error"))
+        self.assertFalse(ApiClient._looks_like_invalid_token_message(""))
+
+    def test_response_invalid_jwt_requires_auth_status_codes(self) -> None:
+        valid_status = _FakeResponse(401, {"detail": "token has expired"})
+        wrong_status = _FakeResponse(404, {"detail": "token has expired"})
+        self.assertTrue(ApiClient(Config())._response_indicates_invalid_jwt(valid_status))
+        self.assertFalse(ApiClient(Config())._response_indicates_invalid_jwt(wrong_status))
+
+    def test_invalidate_session_clears_tokens_even_if_callback_errors(self) -> None:
+        cfg = Config(auth_method="jwt", access_token="a", refresh_token="r")
+        client = ApiClient(cfg, on_token_update=lambda: (_ for _ in ()).throw(RuntimeError("boom")))
+        client._invalidate_jwt_session()
+        self.assertIsNone(cfg.auth_method)
+        self.assertIsNone(cfg.access_token)
+        self.assertIsNone(cfg.refresh_token)
+
+    def test_refresh_access_token_paths(self) -> None:
+        cfg = Config(base_url="https://example.test", auth_method="jwt", refresh_token="ref")
+        update = Mock()
+        client = ApiClient(cfg, on_token_update=update)
+
+        client.session.post = Mock(side_effect=requests.RequestException("network"))
+        self.assertFalse(client._refresh_access_token())
+
+        cfg = Config(base_url="https://example.test", auth_method="jwt", access_token="a", refresh_token="ref")
+        client = ApiClient(cfg, on_token_update=update)
+        client.session.post = Mock(return_value=_FakeResponse(401, {"detail": "Token is invalid or expired"}))
+        self.assertFalse(client._refresh_access_token())
+        self.assertIsNone(cfg.auth_method)
+
+        cfg = Config(base_url="https://example.test", auth_method="jwt", refresh_token="ref")
+        client = ApiClient(cfg, on_token_update=update)
+        client.session.post = Mock(return_value=_FakeResponse(200, ValueError("no json")))
+        self.assertFalse(client._refresh_access_token())
+
+        cfg = Config(base_url="https://example.test", auth_method="jwt", refresh_token="ref")
+        client = ApiClient(cfg, on_token_update=update)
+        client.session.post = Mock(return_value=_FakeResponse(200, {"refresh": "r2"}))
+        self.assertFalse(client._refresh_access_token())
+
+        cfg = Config(base_url="https://example.test", auth_method="jwt", refresh_token="ref")
+        update_ok = Mock()
+        client = ApiClient(cfg, on_token_update=update_ok)
+        client.session.post = Mock(return_value=_FakeResponse(200, {"access": "a2", "refresh": "r2"}))
+        self.assertTrue(client._refresh_access_token())
+        self.assertEqual(cfg.access_token, "a2")
+        self.assertEqual(cfg.refresh_token, "r2")
+        self.assertEqual(cfg.auth_method, "jwt")
+        update_ok.assert_called_once()
+
+    def test_refresh_success_ignores_callback_exception(self) -> None:
+        cfg = Config(base_url="https://example.test", auth_method="jwt", refresh_token="ref")
+        client = ApiClient(cfg, on_token_update=lambda: (_ for _ in ()).throw(RuntimeError("boom")))
+        client.session.post = Mock(return_value=_FakeResponse(200, {"access": "new-token"}))
+        self.assertTrue(client._refresh_access_token())
+        self.assertEqual(cfg.access_token, "new-token")
+
+    def test_refresh_session_only_when_jwt(self) -> None:
+        client = ApiClient(Config(auth_method="api_key", api_key="key"))
+        self.assertFalse(client.refresh_session())
+
+    def test_request_exception_returns_failed_response(self) -> None:
+        client = ApiClient(Config(base_url="https://example.test"))
+        client.session.request = Mock(side_effect=requests.RequestException("timeout"))
+        resp = client.get("/status")
+        self.assertEqual(resp, ApiResponse(False, 0, None, "timeout"))
+
+    def test_request_retries_once_after_refresh_success(self) -> None:
+        cfg = Config(base_url="https://example.test", auth_method="jwt", access_token="old", refresh_token="ref")
+        client = ApiClient(cfg)
+        first = _FakeResponse(401, {"detail": "Token has expired"}, text="unauthorized")
+        second = _FakeResponse(200, {"ok": True}, text='{"ok": true}')
+        client.session.request = Mock(side_effect=[first, second])
+        client._refresh_access_token = Mock(return_value=True)  # type: ignore[assignment]
+
+        resp = client.request("GET", "/x")
+        self.assertTrue(resp.ok)
+        self.assertEqual(resp.status_code, 200)
+        client._refresh_access_token.assert_called_once()
+        self.assertEqual(client.session.request.call_count, 2)
+
+    def test_request_handles_error_text_fallback_and_invalidation(self) -> None:
+        cfg = Config(base_url="https://example.test", auth_method="jwt", access_token="old")
+        client = ApiClient(cfg)
+        client._refresh_access_token = Mock(return_value=False)  # type: ignore[assignment]
+        client.session.request = Mock(return_value=_FakeResponse(403, {"detail": "invalid token"}, text=""))
+        resp = client.post("/x", json_data={"a": 1})
+        self.assertFalse(resp.ok)
+        self.assertEqual(resp.text, "HTTP 403")
+        self.assertIsNone(cfg.auth_method)
+
+    def test_request_merges_extra_headers(self) -> None:
+        cfg = Config(base_url="https://example.test")
+        client = ApiClient(cfg)
+        response = _FakeResponse(200, {"ok": True}, text='{"ok":true}')
+        client.session.request = Mock(return_value=response)
+        client.request("GET", "/x", headers={"X-Test": "1"})
+        called_headers = client.session.request.call_args.kwargs["headers"]
+        self.assertEqual(called_headers["Accept"], "application/json")
+        self.assertEqual(called_headers["X-Test"], "1")
+
+
+if __name__ == "__main__":
+    unittest.main()

tests/test_config.py

@@ -72,6 +72,19 @@ def test_default_config_path_uses_env_override(self) -> None:
             with patch.dict("os.environ", {"DATAPLICITY_CONFIG_DIR": temp_dir}, clear=False):
                 self.assertEqual(default_config_path(), Path(temp_dir) / "cli.json")
 
+    def test_default_config_path_uses_xdg_when_platformdirs_unavailable(self) -> None:
+        with tempfile.TemporaryDirectory() as temp_dir:
+            with patch("dataplicity_cli.config.user_config_dir", None):
+                with patch.dict("os.environ", {"XDG_CONFIG_HOME": temp_dir}, clear=False):
+                    self.assertEqual(default_config_path(), Path(temp_dir) / "dataplicity" / "cli.json")
+
+    def test_default_config_path_falls_back_to_home_config(self) -> None:
+        fake_home = Path("/tmp/fake-home")
+        with patch("dataplicity_cli.config.user_config_dir", None):
+            with patch.dict("os.environ", {}, clear=True):
+                with patch("pathlib.Path.home", return_value=fake_home):
+                    self.assertEqual(default_config_path(), fake_home / ".config" / "dataplicity" / "cli.json")
+
 
 if __name__ == "__main__":
     unittest.main()