From 22a0df0c53976a6f01e77afc910a222f5dc5cf79 Mon Sep 17 00:00:00 2001 From: chengyongru Date: Fri, 15 May 2026 13:42:41 +0800 Subject: [PATCH] =?UTF-8?q?simplify(pairing):=20address=20review=20finding?= =?UTF-8?q?s=20=E2=80=94=20constants,=20TOCTOU,=20nesting?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove TOCTOU exists() check in _load(); rely on FileNotFoundError - Define PAIRING_CODE_META_KEY and PAIRING_COMMAND_META_KEY constants in nanobot.pairing, replacing magic strings across base.py, slack.py, and builtin.py - Flatten nested revoke logic in handle_pairing_command() - Trim redundant docstring/comment noise in is_allowed() and generate_code() --- nanobot/channels/base.py | 16 +++------------- nanobot/channels/slack.py | 4 ++-- nanobot/command/builtin.py | 4 ++-- nanobot/pairing/__init__.py | 6 ++++++ nanobot/pairing/store.py | 15 +++++---------- 5 files changed, 18 insertions(+), 27 deletions(-) diff --git a/nanobot/channels/base.py b/nanobot/channels/base.py index a578e9971..ee523da5a 100644 --- a/nanobot/channels/base.py +++ b/nanobot/channels/base.py @@ -11,6 +11,7 @@ from loguru import logger from nanobot.bus.events import InboundMessage, OutboundMessage from nanobot.bus.queue import MessageBus from nanobot.pairing import ( + PAIRING_CODE_META_KEY, format_pairing_reply, generate_code, is_approved, @@ -181,18 +182,7 @@ class BaseChannel(ABC): return bool(streaming) and type(self).send_delta is not BaseChannel.send_delta def is_allowed(self, sender_id: str) -> bool: - """Check if *sender_id* is permitted. - - Priority: - 1. ``allowFrom: ["*"]`` → allow all. - 2. ``allowFrom`` list → allow if sender_id is present. - 3. Pairing store approved list → allow if previously approved. - 4. Otherwise deny. - - An empty ``allowFrom`` list does not cause a hard exit; instead it - defers to the pairing store so that unknown DM senders can request - access via a pairing code. - """ + """Check sender permission: star > allowlist > pairing store > deny.""" if isinstance(self.config, dict): if "allow_from" in self.config: allow_list = self.config.get("allow_from") or [] @@ -228,7 +218,7 @@ class BaseChannel(ABC): channel=self.name, chat_id=str(chat_id), content=format_pairing_reply(code), - metadata={"_pairing_code": code}, + metadata={PAIRING_CODE_META_KEY: code}, ) ) self.logger.info( diff --git a/nanobot/channels/slack.py b/nanobot/channels/slack.py index 8f55338d6..c6cc79736 100644 --- a/nanobot/channels/slack.py +++ b/nanobot/channels/slack.py @@ -18,7 +18,7 @@ from nanobot.bus.queue import MessageBus from nanobot.channels.base import BaseChannel from nanobot.config.paths import get_media_dir from nanobot.config.schema import Base -from nanobot.pairing import format_pairing_reply, generate_code, is_approved +from nanobot.pairing import PAIRING_CODE_META_KEY, format_pairing_reply, generate_code, is_approved from nanobot.utils.helpers import safe_filename, split_message @@ -351,7 +351,7 @@ class SlackChannel(BaseChannel): channel=self.name, chat_id=chat_id, content=reply, - metadata={"_pairing_code": code}, + metadata={PAIRING_CODE_META_KEY: code}, ) ) return diff --git a/nanobot/command/builtin.py b/nanobot/command/builtin.py index cc15bdf5f..27dbdbe74 100644 --- a/nanobot/command/builtin.py +++ b/nanobot/command/builtin.py @@ -548,14 +548,14 @@ async def cmd_history(ctx: CommandContext) -> OutboundMessage: async def cmd_pairing(ctx: CommandContext) -> OutboundMessage: """List, approve, deny or revoke pairing requests.""" - from nanobot.pairing import handle_pairing_command + from nanobot.pairing import PAIRING_COMMAND_META_KEY, handle_pairing_command reply = handle_pairing_command(ctx.msg.channel, ctx.args) return OutboundMessage( channel=ctx.msg.channel, chat_id=ctx.msg.chat_id, content=reply, - metadata={"_pairing_command": True}, + metadata={PAIRING_COMMAND_META_KEY: True}, ) diff --git a/nanobot/pairing/__init__.py b/nanobot/pairing/__init__.py index 3c62e411a..1650500ee 100644 --- a/nanobot/pairing/__init__.py +++ b/nanobot/pairing/__init__.py @@ -13,6 +13,10 @@ from nanobot.pairing.store import ( revoke, ) +# Metadata keys used by channels and commands to tag pairing-related messages. +PAIRING_CODE_META_KEY = "_pairing_code" +PAIRING_COMMAND_META_KEY = "_pairing_command" + __all__ = [ "approve_code", "deny_code", @@ -24,4 +28,6 @@ __all__ = [ "is_approved", "list_pending", "revoke", + "PAIRING_CODE_META_KEY", + "PAIRING_COMMAND_META_KEY", ] diff --git a/nanobot/pairing/store.py b/nanobot/pairing/store.py index 5ca9c629e..37ac2f4f4 100644 --- a/nanobot/pairing/store.py +++ b/nanobot/pairing/store.py @@ -35,11 +35,11 @@ def _store_path() -> Path: def _load() -> dict[str, Any]: path = _store_path() - if not path.exists(): - return {"approved": {}, "pending": {}} try: with open(path, encoding="utf-8") as f: data = json.load(f) + except FileNotFoundError: + return {"approved": {}, "pending": {}} except (json.JSONDecodeError, OSError): logger.warning("Corrupted pairing store, resetting") return {"approved": {}, "pending": {}} @@ -82,8 +82,6 @@ def generate_code( with _LOCK: data = _load() _gc_pending(data) - # Collision probability is negligible (~1e-12 with 20 pending codes), - # so we skip an existence check for simplicity. raw = "".join(secrets.choice(_ALPHABET) for _ in range(_CODE_LENGTH)) code = f"{raw[:4]}-{raw[4:]}" @@ -236,22 +234,19 @@ def handle_pairing_command(channel: str, subcommand_text: str) -> str: return f"Pairing code `{arg}` not found or already expired" elif sub == "revoke": - if arg is None: - return "Usage: `/pairing revoke ` or `/pairing revoke `" - elif len(parts) == 2: + if len(parts) == 2: return ( f"Revoked {arg} from {channel}" if revoke(channel, arg) else f"{arg} was not in the approved list for {channel}" ) - elif len(parts) == 3: + if len(parts) == 3: return ( f"Revoked {parts[2]} from {arg}" if revoke(arg, parts[2]) else f"{parts[2]} was not in the approved list for {arg}" ) - else: - return "Usage: `/pairing revoke ` or `/pairing revoke `" + return "Usage: `/pairing revoke ` or `/pairing revoke `" return ( "Unknown pairing command.\n"