Skip to content

Reject wallet text control characters#151

Merged
ramimbo merged 1 commit into
ramimbo:mainfrom
YunHeTracyLee:codex/reject-wallet-text-control-chars
May 24, 2026
Merged

Reject wallet text control characters#151
ramimbo merged 1 commit into
ramimbo:mainfrom
YunHeTracyLee:codex/reject-wallet-text-control-chars

Conversation

@YunHeTracyLee
Copy link
Copy Markdown
Contributor

Refs #122

Summary

  • Reject ASCII control characters in optional wallet labels before they are stored and rendered.
  • Reject ASCII control characters in signed transfer memos before they are stored in wallet transfer history.
  • Add service-level and API-level regressions for wallet labels and transfer memos.

Why

Wallet labels and transfer memos are public wallet surfaces, but they only had trimming/length checks. A newline or other ASCII control character could be accepted and stored, unlike the newer public URL and bounty text hardening. This keeps wallet-facing text fields on the same validation footing without changing normal labels or memos.

Verification

  • Direct pre-fix probe accepted label="Main\nWallet" and memo="line1\nline2".
  • python -m pytest tests/test_wallets.py tests/test_wallet_api.py -q -> 29 passed
  • python -m pytest -q -> 148 passed, 2 warnings
  • ruff check app/ledger/service.py tests/test_wallets.py tests/test_wallet_api.py -> All checks passed
  • ruff format --check app/ledger/service.py tests/test_wallets.py tests/test_wallet_api.py -> 3 files already formatted
  • mypy app -> Success: no issues found in 11 source files
  • scripts/docs_smoke.py -> docs smoke ok
  • scripts/check_agents.py -> AGENTS.md ok
  • git diff --check -> passed

Duplicate check:

No secrets, wallet material, deployment values, private context, or MRWK price claims are included.

@YunHeTracyLee
Copy link
Copy Markdown
Contributor Author

I noticed after opening this that PR #150 was opened a few minutes earlier with the same wallet label/memo control-character scope.

This PR differs by checking raw input before trimming and by adding API-level coverage for both register and transfer, which addresses the boundary issue raised on #150. Still, the scope overlaps. Maintainers: please treat #151 as an alternative clean implementation only if useful; if #150 is updated and preferred, I am fine with closing this as duplicate.

Copy link
Copy Markdown

@Ckeplinger199 Ckeplinger199 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers found.

I reviewed the wallet text control-character change in app/ledger/service.py plus the service/API regressions in tests/test_wallets.py and tests/test_wallet_api.py.

The key correctness point is that this PR checks the raw label/memo input before trimming, so boundary control characters are rejected instead of silently normalized away. I verified labels with leading newline, trailing tab, embedded newline, and DEL were rejected, and transfer memos with leading tab, trailing newline, embedded newline, and DEL were rejected.

Verification on head 8ad4f160045488343406161059e2a8f630d44dc2:

  • uv run --extra dev python -m pytest tests/test_wallets.py tests/test_wallet_api.py -q -> 29 passed
  • uv run --extra dev python -m pytest tests/test_security.py tests/test_ledger.py tests/test_wallets.py tests/test_wallet_api.py -q -> 66 passed
  • Ruff format/check on touched files passed
  • uv run --extra dev python -m mypy app -> Success
  • scripts/check_agents.py, scripts/docs_smoke.py, and git diff --check origin/main...HEAD passed

The scope overlaps with PR #150, as the author noted, so choosing between them is a maintainer dedupe question. From this PR's own correctness and verification evidence, I found no blocker.

@ramimbo ramimbo merged commit 2e6c02a into ramimbo:main May 24, 2026
1 check passed
@ramimbo ramimbo added the mrwk:accepted Maintainer accepted for payout label May 24, 2026
@ramimbo ramimbo added the mrwk:paid Ledger payment recorded label May 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mrwk:accepted Maintainer accepted for payout mrwk:paid Ledger payment recorded

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants