Build shared WC24 mail foundation - #7
Conversation
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
Reviewer's GuideIntroduce a shared filesystem-backed WC24 mail service plus Dolphin provisioning, wiring it into the CLI and WC24 config handling, and add a protocol verifier to exercise the complete CGI flow and HMAC behavior. Sequence diagram for mail-config provisioning flowsequenceDiagram
actor User
participant Jwc24CLI as jwc24_main
participant WC24Config as wc24_config
participant Store as MailStore
participant FS as filesystem
User->>Jwc24CLI: invoke mail-config
Jwc24CLI->>WC24Config: read(args.config)
Jwc24CLI->>WC24Config: summarize(config)
Jwc24CLI->>Store: MailStore(args.data_dir)
Jwc24CLI->>Store: register(state.nwc24_id)
Store->>FS: _read_accounts / _write_accounts
Store-->>Jwc24CLI: MailAccount(password, mlchkid)
Jwc24CLI->>WC24Config: configure_mail(args.config, args.base_url, account.password, account.mlchkid, args.apply)
WC24Config->>FS: read / backup / write_bytes
WC24Config-->>Jwc24CLI: (before_urls, after_urls, backup)
Jwc24CLI-->>User: print URL changes and apply/dry-run status
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
6e20a13 to
db04b6b
Compare
db04b6b to
09d9931
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="jwc24/__main__.py" line_range="77" />
<code_context>
+ )
+ for old, new in zip(before, after):
+ print(f"{old} -> {new}")
+ print(f"applied; backup: {backup}" if backup else "dry run only; pass --apply")
+ return 0
if args.command == "account":
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The message conflates a no-op `--apply` run with a dry run, which can be misleading.
In `mail-config`, `configure_mail` returns `backup=None` both when `--apply` is omitted and when `--apply` is used but no changes are needed (`data == original`). Since the message is based solely on `backup is None`, users who did pass `--apply` but had no changes see “dry run only; pass --apply,” which is incorrect. It would be clearer to branch on `apply` and whether changes were made, e.g. distinct messages for: `apply=False`, `apply=True && data==original`, and `apply=True && data!=original`.
Suggested implementation:
```python
account = MailStore(args.data_dir).register(f"{state.nwc24_id:016d}")
before, after, backup = wc24_config.configure_mail(
args.config,
args.base_url,
account.password,
account.mlchkid,
args.apply,
)
for old, new in zip(before, after):
print(f"{old} -> {new}")
# Distinguish between dry-run, no-op apply, and applied changes
changes_made = list(before) != list(after)
if not args.apply:
if changes_made:
print("dry run only; pass --apply to apply these changes")
else:
print("dry run; no changes needed")
elif not changes_made:
print("no changes needed; configuration already up to date")
else:
print(f"applied; backup: {backup}")
return 0
if args.command == "account":
```
```python
if args.apply and not args.bootstrap_local:
raise ValueError("--apply requires --bootstrap-local")
print(f"checksum: {after.calculated_checksum:08x} (valid={after.checksum_valid})")
# Reflect whether this was an apply or a dry-run, and if any changes were needed
if args.apply:
print(f"applied; backup: {backup}" if backup else "applied; no changes needed")
else:
print("dry run; no changes needed")
else:
config = wc24_config.read(args.config)
state = wc24_config.summarize(config)
print(f"WiiConnect24 ID: {state.nwc24_id}")
print(f"ID generation: {state.id_generation}")
```
- In the second block, I don’t see `before`/`after` being set in the snippet; if this branch also has access to them, you may want to mirror the `changes_made = list(before) != list(after)` logic from the first block for consistency and to avoid inferring “no changes” solely from `backup is None`.
- If `configure_mail` (or the code in the `account` branch) can ever produce a non-`None` `backup` for a dry run, you should adjust the second branch’s messages to account for that (e.g., by computing `changes_made` there as well).
</issue_to_address>
### Comment 2
<location path="jwc24/mail_server.py" line_range="148-156" />
<code_context>
+ self.end_headers()
+ self.wfile.write(response)
+ return
+ if path.endswith("/delete.cgi"):
+ form = parse_qs(body.decode("ascii"), keep_blank_values=True)
+ account = store.authenticate(
+ normalize_wii_id(form["mlid"][0]), form["passwd"][0]
+ )
+ if account is None:
+ self._reply(cgi_response(cd=250))
+ return
+ deleted = store.delete_claimed(account)
+ self._reply(cgi_response(cd=100, deletenum=deleted))
+ return
</code_context>
<issue_to_address>
**issue (bug_risk):** `delete.cgi` ignores the `delnum` parameter and always deletes all claimed messages.
`/delete.cgi` parses `delnum` but then calls `store.delete_claimed(account)` without using it, so all `.sent` messages are deleted regardless of the requested count. This matches the current test setup (single message) but conflicts with the semantics of `delnum` and could surprise callers expecting partial deletion. Either remove `delnum` from the request/response if it’s not needed, or implement deletion of up to `delnum` messages to align with the protocol.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| ) | ||
| for old, new in zip(before, after): | ||
| print(f"{old} -> {new}") | ||
| print(f"applied; backup: {backup}" if backup else "dry run only; pass --apply") |
There was a problem hiding this comment.
suggestion (bug_risk): The message conflates a no-op --apply run with a dry run, which can be misleading.
In mail-config, configure_mail returns backup=None both when --apply is omitted and when --apply is used but no changes are needed (data == original). Since the message is based solely on backup is None, users who did pass --apply but had no changes see “dry run only; pass --apply,” which is incorrect. It would be clearer to branch on apply and whether changes were made, e.g. distinct messages for: apply=False, apply=True && data==original, and apply=True && data!=original.
Suggested implementation:
account = MailStore(args.data_dir).register(f"{state.nwc24_id:016d}")
before, after, backup = wc24_config.configure_mail(
args.config,
args.base_url,
account.password,
account.mlchkid,
args.apply,
)
for old, new in zip(before, after):
print(f"{old} -> {new}")
# Distinguish between dry-run, no-op apply, and applied changes
changes_made = list(before) != list(after)
if not args.apply:
if changes_made:
print("dry run only; pass --apply to apply these changes")
else:
print("dry run; no changes needed")
elif not changes_made:
print("no changes needed; configuration already up to date")
else:
print(f"applied; backup: {backup}")
return 0
if args.command == "account": if args.apply and not args.bootstrap_local:
raise ValueError("--apply requires --bootstrap-local")
print(f"checksum: {after.calculated_checksum:08x} (valid={after.checksum_valid})")
# Reflect whether this was an apply or a dry-run, and if any changes were needed
if args.apply:
print(f"applied; backup: {backup}" if backup else "applied; no changes needed")
else:
print("dry run; no changes needed")
else:
config = wc24_config.read(args.config)
state = wc24_config.summarize(config)
print(f"WiiConnect24 ID: {state.nwc24_id}")
print(f"ID generation: {state.id_generation}")- In the second block, I don’t see
before/afterbeing set in the snippet; if this branch also has access to them, you may want to mirror thechanges_made = list(before) != list(after)logic from the first block for consistency and to avoid inferring “no changes” solely frombackup is None. - If
configure_mail(or the code in theaccountbranch) can ever produce a non-Nonebackupfor a dry run, you should adjust the second branch’s messages to account for that (e.g., by computingchanges_madethere as well).
| if path.endswith("/delete.cgi"): | ||
| form = parse_qs(body.decode("ascii"), keep_blank_values=True) | ||
| account = store.authenticate( | ||
| normalize_wii_id(form["mlid"][0]), form["passwd"][0] | ||
| ) | ||
| if account is None: | ||
| self._reply(cgi_response(cd=250)) | ||
| return | ||
| deleted = store.delete_claimed(account) |
There was a problem hiding this comment.
issue (bug_risk): delete.cgi ignores the delnum parameter and always deletes all claimed messages.
/delete.cgi parses delnum but then calls store.delete_claimed(account) without using it, so all .sent messages are deleted regardless of the requested count. This matches the current test setup (single message) but conflicts with the semantics of delnum and could surprise callers expecting partial deletion. Either remove delnum from the request/response if it’s not needed, or implement deletion of up to delnum messages to align with the protocol.
![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 automated review suggestions for this pull request.
Reviewed commit: 09d9931186
ℹ️ 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
- 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 address that feedback".
| form = parse_qs(body.decode("ascii"), keep_blank_values=True) | ||
| account = store.register(normalize_wii_id(form["mlid"][0])) | ||
| self._reply( | ||
| cgi_response(cd=100, passwd=account.password, mlchkid=account.mlchkid) |
There was a problem hiding this comment.
When mail-serve is bound to a non-loopback host, anyone who knows a Wii address can submit its caller-controlled mlid to account.cgi; register() returns the existing record, and this response exposes its password and check ID without validating hdid or any other device credential. Because the Wii address is necessarily shared with correspondents, this lets a remote caller authenticate as the victim to send, receive, or delete mail; existing registrations need authentication or must not return stored credentials.
Useful? React with 👍 / 👎.
| for path in self.pending(account.wii_id)[:limit]: | ||
| payload = path.read_bytes().replace(b"\n", b"\r\n").replace(b"\r\r\n", b"\r\n") | ||
| if claimed and used + len(payload) > max_size: | ||
| break | ||
| if len(payload) > max_size: | ||
| continue | ||
| path.replace(path.with_suffix(".sent")) |
There was a problem hiding this comment.
If the connection closes while receive.cgi is writing its multipart response, this rename has already removed the message from the *.eml set used by both pending() and later claim() calls. Since the client never gets a chance to acknowledge the download with delete.cgi, subsequent checks report no mail and retries cannot retrieve the stranded *.sent message, causing permanent delivery loss; claims must remain retryable or be rolled back when the response is not completed.
Useful? React with 👍 / 👎.
| digest = hashlib.sha256(message_payload).hexdigest() | ||
| for recipient in recipients: | ||
| inbox = self.messages / recipient | ||
| inbox.mkdir(parents=True, exist_ok=True) | ||
| destination = inbox / f"{digest}.eml" | ||
| if not destination.exists(): | ||
| destination.write_bytes(message_payload) |
There was a problem hiding this comment.
When the same serialized MIME payload is sent to a recipient more than once before the first copy is consumed, both deliveries produce the same digest-derived path, so the second is silently skipped while send.cgi still reports cd=100. Identical messages are valid—especially for automated/template mail—and a queue should preserve each accepted delivery rather than deduplicating it by content hash; use a unique per-delivery filename.
Useful? React with 👍 / 👎.
1 participant
What changed
account.cgi, challenge/HMACcheck.cgi, multipartsend.cgi,multipart
receive.cgi, anddelete.cgimail-configprovisioning with checksum repair and backupsmail-serveand expanded account auditing commandsWhy
Wii Mail is shared infrastructure used by the Message Board, friend
registration, and channel features such as TV no Tomo stamp sharing. It should
not be embedded in any one channel adapter.
Validation
cd=100with a valid HMACnwc24msg.cfgchecksum remains valid after provisioningKnown emulator boundary
Dolphin 2606 currently skips incoming WC24 download entries with an empty
destination filename. Server-side receive/delete support is complete, but
moving downloaded MIME into
wc24recv.mbxrequires a Dolphin KD implementationpatch; no channel WAD patch is needed.
Summary by Sourcery
Introduce a shared filesystem-backed WC24 mail service and Dolphin mail provisioning tooling, decoupling Wii Mail from individual channel adapters.
New Features:
Enhancements:
Documentation:
Tests: