security: harden granthi-link + client against 7 codex findings

1. CRITICAL account-takeover by login collision: persist zitadel_sub ->
   gitea_login identity map (state.json, 0600, atomic); mapping wins,
   deleted logins re-created only if service-created, existing unmapped
   logins bind only on verified email match, else 409; token never
   minted before binding passes
2. test_mode now gated behind GRANTHI_LINK_ALLOW_TEST_MODE=1 env
3. refuse startup unless config.json is 0600/0400 and owned by service
4. client config created O_CREAT 0600 (no write-then-chmod window)
5. credential-helper command paths shlex-quoted
6. POST bodies capped at 64KB (413); missing/invalid Content-Length rejected
7. Gitea 409 on user create handled idempotently (re-fetch + verify email)

Co-Authored-By: Claude Fable 5 <[email protected]>

tests/test_client.py

@@ -3,6 +3,7 @@ config handling, device-flow polling (mocked HTTP). Stdlib unittest only."""
 
 import json
 import os
+import shlex
 import shutil
 import subprocess
 import sys
@@ -124,11 +125,65 @@ class TestConfig(unittest.TestCase):
         self.assertEqual(st.st_mode & 0o777, 0o600)
         self.assertEqual(client.load_config()["login"], "x")
 
+    def test_save_is_0600_even_with_permissive_umask(self):
+        """Finding 4: the token file must be born 0600 (O_CREAT mode), not
+        chmod'ed after write -- a wide-open umask must not widen it."""
+        old = os.umask(0o000)
+        try:
+            client.save_config({"token": "sekrit", "folders": {}})
+        finally:
+            os.umask(old)
+        st = os.stat(client.CONFIG_PATH)
+        self.assertEqual(st.st_mode & 0o777, 0o600)
+
+    def test_save_never_calls_chmod(self):
+        """The 0600 mode must come from creation, not a later chmod (which
+        would leave a window where the file is world-readable)."""
+        with mock.patch.object(client.os, "chmod",
+                               side_effect=AssertionError(
+                                   "chmod used; file must be created 0600")):
+            client.save_config({"token": "sekrit", "folders": {}})
+        st = os.stat(client.CONFIG_PATH)
+        self.assertEqual(st.st_mode & 0o777, 0o600)
+
     def test_load_missing_returns_empty(self):
         with mock.patch.object(client, "CONFIG_PATH", "/nonexistent/nope.json"):
             self.assertEqual(client.load_config(), {})
 
 
+class TestCredentialHelperQuoting(unittest.TestCase):
+    """Finding 5: helper command paths must be shlex-quoted."""
+
+    def test_paths_with_spaces_are_quoted(self):
+        with mock.patch.object(client.sys, "executable",
+                               "/opt/py dir/bin/python3"), \
+                mock.patch.object(client, "__file__",
+                                  "/home/a user/granthi sync/client.py"):
+            val = client.credential_helper_value()
+        self.assertTrue(val.startswith("!"))
+        self.assertIn("'/opt/py dir/bin/python3'", val)
+        self.assertIn("'/home/a user/granthi sync/client.py'", val)
+        # shell round-trip yields exactly [python, script, subcommand]
+        parts = shlex.split(val[1:])
+        self.assertEqual(parts, ["/opt/py dir/bin/python3",
+                                 "/home/a user/granthi sync/client.py",
+                                 "git-credential"])
+
+    def test_metacharacters_do_not_inject(self):
+        evil = "/tmp/x; rm -rf ~; echo/client.py"
+        with mock.patch.object(client, "__file__", evil):
+            val = client.credential_helper_value()
+        parts = shlex.split(val[1:])
+        self.assertEqual(parts[1], os.path.abspath(evil))
+        self.assertEqual(len(parts), 3)
+
+    def test_plain_paths_still_work(self):
+        val = client.credential_helper_value()
+        parts = shlex.split(val[1:])
+        self.assertEqual(parts[0], sys.executable)
+        self.assertEqual(parts[2], "git-credential")
+
+
 class TestDeviceFlow(unittest.TestCase):
     def test_device_flow_polls_until_token(self):
         calls = []

tests/test_server.py

@@ -1,12 +1,16 @@
-"""Unit tests for granthi-link: login derivation, link/repos flows against a
-stub HTTP server that plays both Zitadel userinfo and the Gitea API."""
+"""Unit tests for granthi-link: login derivation, identity binding rules,
+link/repos flows against a stub HTTP server that plays both Zitadel
+userinfo and the Gitea API, plus startup/transport hardening."""
 
+import http.client
 import json
 import os
 import sys
+import tempfile
 import threading
 import unittest
 import urllib.request
+from unittest import mock
 from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer
 
 sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "server"))
@@ -14,7 +18,12 @@ import granthi_link  # noqa: E402
 
 
 class StubUpstream(BaseHTTPRequestHandler):
-    """Plays Zitadel (/oidc/v1/userinfo) and Gitea (everything else)."""
+    """Plays Zitadel (/oidc/v1/userinfo) and Gitea (everything else).
+
+    state["users"]: dict login -> email (existing Gitea users)
+    state["hide_once"]: logins whose next GET 404s (simulates a concurrent
+        create racing between the existence check and the create call)
+    """
     state = None  # dict injected per-test
 
     def _json(self, status, obj):
@@ -34,10 +43,14 @@ class StubUpstream(BaseHTTPRequestHandler):
                                         "[email protected]", "email":
                                         "[email protected]", "name": "Alice"})
             return self._json(401, {"error": "invalid token"})
-        if self.path.startswith("/api/v1/users/"):
+        if self.path.startswith("/api/v1/users/") and not self.path.endswith("/tokens"):
             login = self.path.rsplit("/", 1)[1]
+            if login in st["hide_once"]:
+                st["hide_once"].discard(login)
+                return self._json(404, {"message": "not found"})
             if login in st["users"]:
-                return self._json(200, {"login": login})
+                return self._json(200, {"login": login,
+                                        "email": st["users"][login]})
             return self._json(404, {"message": "not found"})
         self._json(404, {})
 
@@ -46,7 +59,9 @@ class StubUpstream(BaseHTTPRequestHandler):
         length = int(self.headers.get("Content-Length", 0))
         body = json.loads(self.rfile.read(length) or b"{}")
         if self.path == "/api/v1/admin/users":
-            st["users"].add(body["username"])
+            if body["username"] in st["users"]:
+                return self._json(409, {"message": "user already exists"})
+            st["users"][body["username"]] = body["email"]
             st["created"].append(body)
             return self._json(201, {"login": body["username"]})
         if self.path.startswith("/api/v1/users/") and self.path.endswith("/tokens"):
@@ -72,20 +87,42 @@ class StubUpstream(BaseHTTPRequestHandler):
 
 class ServiceTestBase(unittest.TestCase):
     def setUp(self):
-        StubUpstream.state = {"users": set(), "created": [], "repos": set(),
-                              "token_reqs": []}
+        StubUpstream.state = {"users": {}, "created": [], "repos": set(),
+                              "token_reqs": [], "hide_once": set()}
         self.upstream = ThreadingHTTPServer(("127.0.0.1", 0), StubUpstream)
         threading.Thread(target=self.upstream.serve_forever, daemon=True).start()
         self.addCleanup(self.upstream.shutdown)
         base = f"http://127.0.0.1:{self.upstream.server_address[1]}"
+        self.state_dir = tempfile.mkdtemp(prefix="granthi-link-state-")
+        self.state_path = os.path.join(self.state_dir, "state.json")
         self.svc = granthi_link.LinkService({
             "gitea_base": base,
             "public_gitea_base": "http://public.example:3041",
             "zitadel_userinfo": f"{base}/oidc/v1/userinfo",
             "admin_token": "ADMTOK", "admin_login": "root",
             "admin_password": "rootpw", "test_mode": False,
+            "state_path": self.state_path,
         })
 
+    def enable_test_mode(self):
+        self.svc.cfg["test_mode"] = True
+        patcher = mock.patch.dict(
+            os.environ, {granthi_link.TEST_MODE_ENV: "1"})
+        patcher.start()
+        self.addCleanup(patcher.stop)
+
+    def stub_link(self, sub, username, email=None, verified=None, device="d"):
+        ui = {"sub": sub, "preferred_username": username}
+        if email is not None:
+            ui["email"] = email
+        if verified is not None:
+            ui["email_verified"] = verified
+        return self.svc.link({"test_userinfo": ui, "device_name": device})
+
+    def read_state(self):
+        with open(self.state_path) as f:
+            return json.load(f)
+
 
 class TestDeriveLogin(unittest.TestCase):
     def test_strips_domain_and_sanitizes(self):
@@ -123,12 +160,15 @@ class TestLink(ServiceTestBase):
         self.assertEqual(sorted(req["body"]["scopes"]),
                          ["write:repository", "write:user"])
 
-    def test_link_existing_user_skips_create(self):
-        StubUpstream.state["users"].add("alice.smith")
-        status, resp = self.svc.link({"zitadel_access_token": "good-token",
-                                      "device_name": "d"})
-        self.assertEqual(status, 200)
-        self.assertEqual(StubUpstream.state["created"], [])
+    def test_link_records_identity_mapping(self):
+        self.svc.link({"zitadel_access_token": "good-token",
+                       "device_name": "d"})
+        state = self.read_state()
+        rec = state["identities"]["123"]
+        self.assertEqual(rec["login"], "alice.smith")
+        self.assertTrue(rec["created_by_service"])
+        mode = os.stat(self.state_path).st_mode & 0o777
+        self.assertEqual(mode, 0o600)
 
     def test_link_bad_token_401(self):
         status, resp = self.svc.link({"zitadel_access_token": "bad",
@@ -139,17 +179,251 @@ class TestLink(ServiceTestBase):
         status, _ = self.svc.link({"device_name": "d"})
         self.assertEqual(status, 400)
 
-    def test_test_mode_stub_only_when_enabled(self):
-        # disabled -> stub ignored, token required
-        status, _ = self.svc.link({"test_userinfo": {"sub": "1",
-                                                     "preferred_username": "x"}})
-        self.assertEqual(status, 400)
+    def test_link_missing_sub_422(self):
+        self.enable_test_mode()
+        status, _ = self.svc.link({"test_userinfo":
+                                   {"preferred_username": "nosub"},
+                                   "device_name": "d"})
+        self.assertEqual(status, 422)
+
+
+class TestIdentityBinding(ServiceTestBase):
+    """Finding 1: account takeover by login collision."""
+
+    def setUp(self):
+        super().setUp()
+        self.enable_test_mode()
+
+    def test_repeat_link_same_sub_reuses_mapping(self):
+        status, resp = self.stub_link("s1", "alice")
+        self.assertEqual(status, 200)
+        # same sub again -- even with a different preferred_username the
+        # mapping wins and no second user is created
+        status, resp = self.stub_link("s1", "totally-different")
+        self.assertEqual(status, 200)
+        self.assertEqual(resp["login"], "alice")
+        self.assertEqual(len(StubUpstream.state["created"]), 1)
+
+    def test_colliding_username_different_sub_409_no_token(self):
+        status, _ = self.stub_link("s1", "alice")
+        self.assertEqual(status, 200)
+        minted_before = len(StubUpstream.state["token_reqs"])
+        status, resp = self.stub_link("s2", "alice")  # attacker
+        self.assertEqual(status, 409)
+        self.assertIn("not linked to this identity", resp["error"])
+        # no token minted for the refused identity
+        self.assertEqual(len(StubUpstream.state["token_reqs"]), minted_before)
+        self.assertNotIn("s2", self.read_state()["identities"])
+
+    def test_existing_user_binds_on_verified_email_match(self):
+        StubUpstream.state["users"]["bob"] = "[email protected]"
+        status, resp = self.stub_link("s9", "bob", email="[email protected]",
+                                      verified=True)
+        self.assertEqual(status, 200)
+        self.assertEqual(resp["login"], "bob")
+        rec = self.read_state()["identities"]["s9"]
+        self.assertFalse(rec["created_by_service"])
+
+    def test_existing_user_unverified_email_409(self):
+        StubUpstream.state["users"]["bob"] = "[email protected]"
+        status, _ = self.stub_link("s9", "bob", email="[email protected]",
+                                   verified=False)
+        self.assertEqual(status, 409)
+        self.assertEqual(StubUpstream.state["token_reqs"], [])
+
+    def test_existing_user_wrong_email_409(self):
+        StubUpstream.state["users"]["bob"] = "[email protected]"
+        status, _ = self.stub_link("s9", "bob", email="[email protected]",
+                                   verified=True)
+        self.assertEqual(status, 409)
+        self.assertEqual(StubUpstream.state["token_reqs"], [])
+
+    def test_deleted_service_created_login_is_recreated(self):
+        self.stub_link("s1", "alice")
+        del StubUpstream.state["users"]["alice"]  # user deleted in Gitea
+        status, resp = self.stub_link("s1", "alice")
+        self.assertEqual(status, 200)
+        self.assertEqual(resp["login"], "alice")
+        self.assertIn("alice", StubUpstream.state["users"])
+
+    def test_deleted_adopted_login_is_refused(self):
+        StubUpstream.state["users"]["bob"] = "[email protected]"
+        status, _ = self.stub_link("s9", "bob", email="[email protected]",
+                                   verified=True)
+        self.assertEqual(status, 200)
+        del StubUpstream.state["users"]["bob"]
+        status, resp = self.stub_link("s9", "bob", email="[email protected]",
+                                      verified=True)
+        self.assertEqual(status, 409)
+        self.assertIn("not created by this service", resp["error"])
+
+    def test_corrupt_state_fails_closed(self):
+        with open(self.state_path, "w") as f:
+            f.write("{ not json")
+        status, resp = self.svc.link({"test_userinfo":
+                                      {"sub": "s1",
+                                       "preferred_username": "alice"},
+                                      "device_name": "d"})
+        self.assertEqual(status, 500)
+        self.assertEqual(StubUpstream.state["token_reqs"], [])
+
+
+class TestConcurrentCreateRace(ServiceTestBase):
+    """Finding 7: Gitea 409 on user create is handled idempotently."""
+
+    def setUp(self):
+        super().setUp()
+        self.enable_test_mode()
+
+    def test_409_on_create_refetches_and_continues(self):
+        # user exists (created by a concurrent request with OUR email) but
+        # the first existence check misses it
+        email = "[email protected]"
+        StubUpstream.state["users"]["alice"] = email
+        StubUpstream.state["hide_once"].add("alice")
+        status, resp = self.stub_link("s1", "alice", email=email)
+        self.assertEqual(status, 200)
+        self.assertEqual(resp["login"], "alice")
+        self.assertEqual(self.read_state()["identities"]["s1"]["login"],
+                         "alice")
+
+    def test_409_on_create_with_foreign_email_refused(self):
+        StubUpstream.state["users"]["alice"] = "[email protected]"
+        StubUpstream.state["hide_once"].add("alice")
+        status, resp = self.stub_link("s1", "alice",
+                                      email="[email protected]")
+        self.assertEqual(status, 409)
+        self.assertEqual(StubUpstream.state["token_reqs"], [])
+
+
+class TestTestModeGate(ServiceTestBase):
+    """Finding 2: test_mode requires the env gate."""
+
+    def test_config_flag_alone_is_ignored(self):
         self.svc.cfg["test_mode"] = True
-        status, resp = self.svc.link({"test_userinfo": {
-            "sub": "1", "preferred_username": "evetest"}, "device_name": "d"})
+        env = {k: v for k, v in os.environ.items()
+               if k != granthi_link.TEST_MODE_ENV}
+        with mock.patch.dict(os.environ, env, clear=True), \
+                self.assertLogs("granthi-link", level="ERROR"):
+            status, _ = self.svc.link({"test_userinfo": {
+                "sub": "1", "preferred_username": "x"}})
+        self.assertEqual(status, 400)  # falls through to token-required
+
+    def test_env_gate_wrong_value_is_ignored(self):
+        self.svc.cfg["test_mode"] = True
+        with mock.patch.dict(os.environ,
+                             {granthi_link.TEST_MODE_ENV: "true"}):
+            status, _ = self.svc.link({"test_userinfo": {
+                "sub": "1", "preferred_username": "x"}})
+        self.assertEqual(status, 400)
+
+    def test_enabled_with_config_and_env(self):
+        self.enable_test_mode()
+        status, resp = self.stub_link("1", "evetest")
         self.assertEqual(status, 200)
         self.assertEqual(resp["login"], "evetest")
 
+    def test_env_alone_without_config_flag_disabled(self):
+        with mock.patch.dict(os.environ,
+                             {granthi_link.TEST_MODE_ENV: "1"}):
+            status, _ = self.svc.link({"test_userinfo": {
+                "sub": "1", "preferred_username": "x"}})
+        self.assertEqual(status, 400)
+
+
+class TestConfigPerms(unittest.TestCase):
+    """Finding 3: refuse startup on permissive or foreign-owned config."""
+
+    def setUp(self):
+        self.tmp = tempfile.NamedTemporaryFile(delete=False, suffix=".json")
+        self.tmp.write(b"{}")
+        self.tmp.close()
+        self.addCleanup(os.unlink, self.tmp.name)
+
+    def test_0600_ok(self):
+        os.chmod(self.tmp.name, 0o600)
+        self.assertIsNone(granthi_link.check_config_perms(self.tmp.name))
+
+    def test_0400_ok(self):
+        os.chmod(self.tmp.name, 0o400)
+        self.assertIsNone(granthi_link.check_config_perms(self.tmp.name))
+
+    def test_0644_refused(self):
+        os.chmod(self.tmp.name, 0o644)
+        err = granthi_link.check_config_perms(self.tmp.name)
+        self.assertIn("refusing to start", err)
+        self.assertIn("0o644", err)
+
+    def test_0640_refused(self):
+        os.chmod(self.tmp.name, 0o640)
+        self.assertIsNotNone(granthi_link.check_config_perms(self.tmp.name))
+
+    def test_foreign_owner_refused(self):
+        os.chmod(self.tmp.name, 0o600)
+        not_me = os.geteuid() + 1
+        err = granthi_link.check_config_perms(self.tmp.name, euid=not_me)
+        self.assertIn("owned by uid", err)
+
+
+class HandlerTestBase(ServiceTestBase):
+    def setUp(self):
+        super().setUp()
+        granthi_link.Handler.service = self.svc
+        self.srv = ThreadingHTTPServer(("127.0.0.1", 0), granthi_link.Handler)
+        threading.Thread(target=self.srv.serve_forever, daemon=True).start()
+        self.addCleanup(self.srv.shutdown)
+        self.port = self.srv.server_address[1]
+
+    def raw_post(self, path, body_bytes=None, headers=None):
+        conn = http.client.HTTPConnection("127.0.0.1", self.port, timeout=10)
+        self.addCleanup(conn.close)
+        conn.putrequest("POST", path)
+        for k, v in (headers or {}).items():
+            conn.putheader(k, v)
+        conn.endheaders()
+        if body_bytes:
+            conn.send(body_bytes)
+        resp = conn.getresponse()
+        return resp.status, json.loads(resp.read() or b"{}")
+
+
+class TestBodyLimits(HandlerTestBase):
+    """Finding 6: bounded reads, Content-Length required on POST."""
+
+    def test_oversized_content_length_413(self):
+        status, resp = self.raw_post(
+            "/v1/link", headers={"Content-Length":
+                                 str(granthi_link.MAX_BODY_BYTES + 1)})
+        self.assertEqual(status, 413)
+        self.assertIn("too large", resp["error"])
+
+    def test_missing_content_length_411(self):
+        status, _ = self.raw_post("/v1/link")
+        self.assertEqual(status, 411)
+
+    def test_invalid_content_length_400(self):
+        status, _ = self.raw_post("/v1/link",
+                                  headers={"Content-Length": "banana"})
+        self.assertEqual(status, 400)
+
+    def test_normal_post_still_works(self):
+        body = json.dumps({"device_name": "d"}).encode()
+        status, resp = self.raw_post(
+            "/v1/link", body_bytes=body,
+            headers={"Content-Length": str(len(body)),
+                     "Content-Type": "application/json"})
+        self.assertEqual(status, 400)  # missing token, but parsed fine
+        self.assertIn("zitadel_access_token", resp["error"])
+
+    def test_at_limit_accepted(self):
+        pad = "x" * (granthi_link.MAX_BODY_BYTES - 30)
+        body = json.dumps({"pad": pad}).encode()
+        self.assertLessEqual(len(body), granthi_link.MAX_BODY_BYTES)
+        status, _ = self.raw_post(
+            "/v1/link", body_bytes=body,
+            headers={"Content-Length": str(len(body))})
+        self.assertEqual(status, 400)  # parsed; fails on missing token
+
 
 class TestRepos(ServiceTestBase):
     def test_repo_create_returns_public_clone_url(self):
@@ -169,14 +443,10 @@ class TestRepos(ServiceTestBase):
         self.assertEqual(status, 400)
 
 
-class TestHealth(ServiceTestBase):
+class TestHealth(HandlerTestBase):
     def test_health_endpoint(self):
-        granthi_link.Handler.service = self.svc
-        srv = ThreadingHTTPServer(("127.0.0.1", 0), granthi_link.Handler)
-        threading.Thread(target=srv.serve_forever, daemon=True).start()
-        self.addCleanup(srv.shutdown)
         with urllib.request.urlopen(
-                f"http://127.0.0.1:{srv.server_address[1]}/health") as r:
+                f"http://127.0.0.1:{self.port}/health") as r:
             body = json.loads(r.read())
         self.assertEqual(body["status"], "ok")
         self.assertEqual(body["service"], "granthi-link")