-
Notifications
You must be signed in to change notification settings - Fork 0
exec: resets + security auth bans parity gating #81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".
| def _parse_host_pattern(raw: str | None) -> tuple[str, BanFlag]: | ||
| text = (raw or "").strip().lower() | ||
| flags = BanFlag(0) | ||
| if text.startswith("*"): | ||
| flags |= BanFlag.PREFIX | ||
| text = text[1:] | ||
| if text.endswith("*"): | ||
| flags |= BanFlag.SUFFIX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P1] Prefix/suffix bans persisted with reversed ROM flags
Leading and trailing wildcards are mapped to BanFlag.PREFIX and BanFlag.SUFFIX respectively, but PREFIX is encoded as the ROM letter "B" and SUFFIX as "A", while the matcher treats PREFIX as an ends-with check and SUFFIX as starts-with. Consequently a suffix ban such as *example.com is written with the "B" prefix flag and a prefix ban like corp.* is written with "A", the opposite of ROM’s format. Any ban file exchanged with a ROM server (or reloaded by ROM tools) would interpret these flags backwards, allowing or blocking the wrong hosts. Swap the flag assignments so leading wildcards produce BanFlag.SUFFIX and trailing wildcards produce BanFlag.PREFIX before persisting.
Useful? React with 👍 / 👎.
| if host_for_ban and bans.is_host_banned(host_for_ban, bans.BanFlag.ALL): | ||
| writer.write(b"Your site has been banned from this mud.\r\n") | ||
| await writer.drain() | ||
| return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P1] Close connection when rejecting BAN_ALL hosts
The BAN_ALL check at the top of handle_connection writes a message and returns before entering the try…finally block. Because the socket cleanup and session removal happen in the finally, this early return leaves the TCP stream open and bypasses standard teardown and logging, allowing a banned client to keep a connection alive indefinitely. Close the writer (and await wait_closed) or move the check inside the guarded block so the normal cleanup path runs.
Useful? React with 👍 / 👎.
Summary
Testing
ruff check .(fails: repository includes shell scripts and pre-existing lint issues)ruff format --check .(fails: repo-wide formatting drift and shell script parsing errors)mypy --strict .(fails: scripts/agent_loop.py is a shell script and cannot be parsed)pytest -q(fails: test environment cannot import the mud package)https://chatgpt.com/codex/tasks/task_b_68ca1ddba04c83209889a29835028dfa