From 7d10fb62609f26e9f9d65e6ceb66cb566afd59b7 Mon Sep 17 00:00:00 2001 From: SurprisedDuck Date: Mon, 1 Jun 2026 22:58:58 +0200 Subject: [PATCH] Reserve internal sentinel usernames MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `core.middleware.require_admin` grants admin to any request whose `request.state.current_user == "internal-tool"` — the sentinel meant only for the in-process tool-loopback path. But the normal cookie auth path (app.py) sets `current_user` to the raw username, and neither `create_user` nor the signup route reserved that name. As a result an account literally named "internal-tool" was silently treated as admin by every `require_admin`-gated route. With self-service signup enabled this is an anonymous -> admin privilege escalation. Reserve the full synthetic-owner set the codebase already special-cases — "internal-tool", "api", "demo", "system" (see `_SYNTHETIC_OWNERS` in routes/assistant_routes.py and the matching guards in src/task_scheduler.py and routes/research_routes.py). "api" collides with the bearer-token owner sentinel; "demo"/"system" would leave a real account denied an assistant and inconsistently owner-scoped. Refuse to create or rename into any reserved name (case/space-normalized), and reject empty usernames while we're here. Adds a regression test. Co-authored-by: Claude --- core/auth.py | 24 +++++++ ...test_reserved_username_admin_escalation.py | 66 +++++++++++++++++++ 2 files changed, 90 insertions(+) create mode 100644 tests/test_reserved_username_admin_escalation.py diff --git a/core/auth.py b/core/auth.py index 953704f..57ca97b 100644 --- a/core/auth.py +++ b/core/auth.py @@ -40,6 +40,22 @@ DEFAULT_AUTH_PATH = os.path.join( ) TOKEN_TTL = 60 * 60 * 24 * 7 # 7 days +# Usernames the auth + middleware layer reserve as internal "synthetic owner" +# sentinels; they must never belong to a real account. The most dangerous is +# "internal-tool": `core.middleware.require_admin` treats any request whose +# `current_user == "internal-tool"` as the in-process tool loopback and grants +# admin, and because the cookie auth path sets `current_user` to the raw +# username, an account literally named "internal-tool" would be silently +# treated as an admin by every `require_admin`-gated route. "api" collides with +# the bearer-token owner-attribution sentinel. "demo"/"system" round out the +# synthetic-owner set the rest of the codebase already special-cases (see +# `_SYNTHETIC_OWNERS` in routes/assistant_routes.py and the matching guards in +# src/task_scheduler.py / routes/research_routes.py) — a real account with one +# of those names would be denied an assistant and inconsistently owner-scoped. +# Refuse to create or rename into any of them so the sentinels can't be +# impersonated. (Keep this in sync with that synthetic-owner set.) +RESERVED_USERNAMES = frozenset({"internal-tool", "api", "demo", "system"}) + def _hash_password(password: str) -> str: return bcrypt.hashpw(password.encode("utf-8"), bcrypt.gensalt()).decode("utf-8") @@ -177,6 +193,11 @@ class AuthManager: def create_user(self, username: str, password: str, is_admin: bool = False) -> bool: """Create a new user account.""" username = username.strip().lower() + if not username: + return False + if username in RESERVED_USERNAMES: + logger.warning("Refused to create reserved username '%s'", username) + return False if username in self.users: return False if "users" not in self._config: @@ -230,6 +251,9 @@ class AuthManager: requesting_user = (requesting_user or "").strip().lower() if not old_username or not new_username: return False + if new_username in RESERVED_USERNAMES: + logger.warning("Refused to rename '%s' into reserved username '%s'", old_username, new_username) + return False if old_username not in self.users: return False if new_username in self.users: diff --git a/tests/test_reserved_username_admin_escalation.py b/tests/test_reserved_username_admin_escalation.py new file mode 100644 index 0000000..e363c02 --- /dev/null +++ b/tests/test_reserved_username_admin_escalation.py @@ -0,0 +1,66 @@ +"""Regression: reserved sentinel usernames must not be registerable. + +`core.middleware.require_admin` grants admin to any request whose +`current_user == "internal-tool"` (the in-process tool-loopback sentinel), +and the cookie auth path in app.py sets `current_user` to the raw username. +Before this fix nothing reserved that name, so a self-service signup (or an +admin typo) creating the account "internal-tool" was silently treated as an +admin by every `require_admin`-gated route — a privilege escalation. "api" +is reserved for the same reason (bearer-token owner attribution collision). + +See the privilege-escalation finding from the 2026-06 code review. +""" + +import sys + +import pytest + + +def _fresh_auth_manager(tmp_path): + # Same import dance as test_security_regressions: drop any cached stub so + # we exercise the real module from disk rather than a conftest mock. + sys.modules.pop("core.auth", None) + if "core" in sys.modules and hasattr(sys.modules["core"], "auth"): + delattr(sys.modules["core"], "auth") + from core.auth import AuthManager + + return AuthManager(str(tmp_path / "auth.json")) + + +@pytest.mark.parametrize( + "name", + ["internal-tool", "api", "demo", "system", "INTERNAL-TOOL", " Internal-Tool ", "Api", "SYSTEM"], +) +def test_create_user_rejects_reserved_usernames(tmp_path, name): + mgr = _fresh_auth_manager(tmp_path) + assert mgr.create_user(name, "pw-123456") is False + # The normalized name must not have been written to the user table. + assert name.strip().lower() not in mgr.users + + +def test_create_user_rejects_empty_username(tmp_path): + mgr = _fresh_auth_manager(tmp_path) + assert mgr.create_user(" ", "pw-123456") is False + assert "" not in mgr.users + + +def test_setup_rejects_reserved_admin_username(tmp_path): + mgr = _fresh_auth_manager(tmp_path) + # First-run admin setup funnels through create_user, so it's covered too. + assert mgr.setup("internal-tool", "pw-123456") is False + assert mgr.is_configured is False + + +def test_rename_into_reserved_username_is_blocked(tmp_path): + mgr = _fresh_auth_manager(tmp_path) + assert mgr.create_user("admin", "pw-123456", is_admin=True) is True + assert mgr.create_user("bob", "pw-123456") is True + assert mgr.rename_user("bob", "internal-tool", "admin") is False + assert "internal-tool" not in mgr.users + assert "bob" in mgr.users + + +def test_normal_usernames_still_allowed(tmp_path): + mgr = _fresh_auth_manager(tmp_path) + assert mgr.create_user("alice", "pw-123456") is True + assert "alice" in mgr.users