simplify(pairing): address review findings — constants, TOCTOU, nesting

- 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()
This commit is contained in:
chengyongru 2026-05-15 13:42:41 +08:00 committed by Xubin Ren
parent b9522e0a4d
commit 22a0df0c53
5 changed files with 18 additions and 27 deletions

View File

@ -11,6 +11,7 @@ from loguru import logger
from nanobot.bus.events import InboundMessage, OutboundMessage from nanobot.bus.events import InboundMessage, OutboundMessage
from nanobot.bus.queue import MessageBus from nanobot.bus.queue import MessageBus
from nanobot.pairing import ( from nanobot.pairing import (
PAIRING_CODE_META_KEY,
format_pairing_reply, format_pairing_reply,
generate_code, generate_code,
is_approved, is_approved,
@ -181,18 +182,7 @@ class BaseChannel(ABC):
return bool(streaming) and type(self).send_delta is not BaseChannel.send_delta return bool(streaming) and type(self).send_delta is not BaseChannel.send_delta
def is_allowed(self, sender_id: str) -> bool: def is_allowed(self, sender_id: str) -> bool:
"""Check if *sender_id* is permitted. """Check sender permission: star > allowlist > pairing store > deny."""
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.
"""
if isinstance(self.config, dict): if isinstance(self.config, dict):
if "allow_from" in self.config: if "allow_from" in self.config:
allow_list = self.config.get("allow_from") or [] allow_list = self.config.get("allow_from") or []
@ -228,7 +218,7 @@ class BaseChannel(ABC):
channel=self.name, channel=self.name,
chat_id=str(chat_id), chat_id=str(chat_id),
content=format_pairing_reply(code), content=format_pairing_reply(code),
metadata={"_pairing_code": code}, metadata={PAIRING_CODE_META_KEY: code},
) )
) )
self.logger.info( self.logger.info(

View File

@ -18,7 +18,7 @@ from nanobot.bus.queue import MessageBus
from nanobot.channels.base import BaseChannel from nanobot.channels.base import BaseChannel
from nanobot.config.paths import get_media_dir from nanobot.config.paths import get_media_dir
from nanobot.config.schema import Base 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 from nanobot.utils.helpers import safe_filename, split_message
@ -351,7 +351,7 @@ class SlackChannel(BaseChannel):
channel=self.name, channel=self.name,
chat_id=chat_id, chat_id=chat_id,
content=reply, content=reply,
metadata={"_pairing_code": code}, metadata={PAIRING_CODE_META_KEY: code},
) )
) )
return return

View File

@ -548,14 +548,14 @@ async def cmd_history(ctx: CommandContext) -> OutboundMessage:
async def cmd_pairing(ctx: CommandContext) -> OutboundMessage: async def cmd_pairing(ctx: CommandContext) -> OutboundMessage:
"""List, approve, deny or revoke pairing requests.""" """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) reply = handle_pairing_command(ctx.msg.channel, ctx.args)
return OutboundMessage( return OutboundMessage(
channel=ctx.msg.channel, channel=ctx.msg.channel,
chat_id=ctx.msg.chat_id, chat_id=ctx.msg.chat_id,
content=reply, content=reply,
metadata={"_pairing_command": True}, metadata={PAIRING_COMMAND_META_KEY: True},
) )

View File

@ -13,6 +13,10 @@ from nanobot.pairing.store import (
revoke, 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__ = [ __all__ = [
"approve_code", "approve_code",
"deny_code", "deny_code",
@ -24,4 +28,6 @@ __all__ = [
"is_approved", "is_approved",
"list_pending", "list_pending",
"revoke", "revoke",
"PAIRING_CODE_META_KEY",
"PAIRING_COMMAND_META_KEY",
] ]

View File

@ -35,11 +35,11 @@ def _store_path() -> Path:
def _load() -> dict[str, Any]: def _load() -> dict[str, Any]:
path = _store_path() path = _store_path()
if not path.exists():
return {"approved": {}, "pending": {}}
try: try:
with open(path, encoding="utf-8") as f: with open(path, encoding="utf-8") as f:
data = json.load(f) data = json.load(f)
except FileNotFoundError:
return {"approved": {}, "pending": {}}
except (json.JSONDecodeError, OSError): except (json.JSONDecodeError, OSError):
logger.warning("Corrupted pairing store, resetting") logger.warning("Corrupted pairing store, resetting")
return {"approved": {}, "pending": {}} return {"approved": {}, "pending": {}}
@ -82,8 +82,6 @@ def generate_code(
with _LOCK: with _LOCK:
data = _load() data = _load()
_gc_pending(data) _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)) raw = "".join(secrets.choice(_ALPHABET) for _ in range(_CODE_LENGTH))
code = f"{raw[:4]}-{raw[4:]}" code = f"{raw[:4]}-{raw[4:]}"
@ -236,21 +234,18 @@ def handle_pairing_command(channel: str, subcommand_text: str) -> str:
return f"Pairing code `{arg}` not found or already expired" return f"Pairing code `{arg}` not found or already expired"
elif sub == "revoke": elif sub == "revoke":
if arg is None: if len(parts) == 2:
return "Usage: `/pairing revoke <user_id>` or `/pairing revoke <channel> <user_id>`"
elif len(parts) == 2:
return ( return (
f"Revoked {arg} from {channel}" f"Revoked {arg} from {channel}"
if revoke(channel, arg) if revoke(channel, arg)
else f"{arg} was not in the approved list for {channel}" else f"{arg} was not in the approved list for {channel}"
) )
elif len(parts) == 3: if len(parts) == 3:
return ( return (
f"Revoked {parts[2]} from {arg}" f"Revoked {parts[2]} from {arg}"
if revoke(arg, parts[2]) if revoke(arg, parts[2])
else f"{parts[2]} was not in the approved list for {arg}" else f"{parts[2]} was not in the approved list for {arg}"
) )
else:
return "Usage: `/pairing revoke <user_id>` or `/pairing revoke <channel> <user_id>`" return "Usage: `/pairing revoke <user_id>` or `/pairing revoke <channel> <user_id>`"
return ( return (