exec: world_loader-security_auth_bans — complete JSON tests and ban persistence #78
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
| permit_host = bool(host and bans.is_host_banned(host, BanFlag.PERMIT)) | ||
| if host and not permit_host and bans.is_host_banned(host, BanFlag.ALL): | ||
| return None | ||
| session = SessionLocal() | ||
| try: | ||
| exists = ( | ||
| session.query(PlayerAccount).filter_by(username=username).first() | ||
| is not None | ||
| ) | ||
| finally: | ||
| session.close() | ||
| if host and not permit_host and not exists and bans.is_host_banned(host, BanFlag.NEWBIES): | ||
| return None |
There was a problem hiding this comment.
[P1] Treat BAN_PERMIT as a whitelist rather than a restricted ban
The new login flow treats a BAN_PERMIT entry as a free pass. login_with_host() sets permit_host when is_host_banned(host, BanFlag.PERMIT) returns true and then bypasses both BAN_ALL and BAN_NEWBIES checks for that connection. In ROM 2.4, check_ban(d->host, BAN_PERMIT) denies access unless the player has the PLR_PERMIT bit (see src/nanny.c around lines 207‑213). Because no player‑side permit flag is checked here, adding a BAN_PERMIT row now allows anyone from that site to log in instead of restricting it to explicitly permitted players, reversing the original security semantics.
Useful? React with 👍 / 👎.
| def _mark_shopkeeper_inventory(mob: MobInstance, obj: object) -> None: | ||
| """Ensure shopkeeper inventory copies carry ITEM_INVENTORY like ROM.""" | ||
|
|
||
| proto = getattr(mob, "prototype", None) | ||
| if getattr(proto, "vnum", None) not in shop_registry: | ||
| return | ||
|
|
||
| item_proto = getattr(obj, "prototype", None) | ||
| if item_proto is None or not hasattr(item_proto, "extra_flags"): | ||
| return | ||
|
|
||
| extra_flags = getattr(item_proto, "extra_flags", 0) | ||
| if isinstance(extra_flags, str): | ||
| extra_flags = convert_flags_from_letters(extra_flags, ExtraFlag) | ||
|
|
||
| item_proto.extra_flags = int(extra_flags) | int(ITEM_INVENTORY) | ||
| setattr(obj, "extra_flags", int(item_proto.extra_flags)) |
There was a problem hiding this comment.
[P1] Shop inventory flag leaks onto object prototypes
_mark_shopkeeper_inventory() ORs ITEM_INVENTORY directly into the prototype’s extra_flags before copying it onto the spawned object. ROM sets this flag on the created object only (src/db.c lines 1858‑1920); the prototype stays unchanged so other spawns of the same item aren’t marked as inventory. By mutating the prototype here, every later copy of that object—whether found in the world or sold elsewhere—will inherit the inventory flag, altering drop behaviour and shop pricing globally.
Useful? React with 👍 / 👎.
Summary
Testing
ruff check .(fails: repository contains legacy shell scripts that ruff parses as Python, e.g. scripts/agent_loop.py)ruff format --check .(fails: same baseline formatting/syntax issues across the tree)mypy --strict .(fails: scripts/agent_loop.py contains shell syntax that mypy cannot parse)pytest -q(fails during collection with baseline ModuleNotFoundError for the mud package)https://chatgpt.com/codex/tasks/task_b_68ca6de7b50483208650233fd10ed6c4