Skip to content

Refs #576: Reject control chars in wallet type filters#609

Open
Squirbie wants to merge 1 commit into
ramimbo:mainfrom
Squirbie:codex/wallet-transaction-type-control-576
Open

Refs #576: Reject control chars in wallet type filters#609
Squirbie wants to merge 1 commit into
ramimbo:mainfrom
Squirbie:codex/wallet-transaction-type-control-576

Conversation

@Squirbie
Copy link
Copy Markdown
Contributor

@Squirbie Squirbie commented May 29, 2026

Summary

  • Reject control characters in the public wallet detail type filter before trimming/rendering.
  • Keep existing clean type-filter behavior unchanged, including unknown clean types rendering an empty filtered state.
  • Add a wallet page regression for type=%09.

Validation

  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest tests/test_wallet_api.py::test_wallet_pages_expose_transfer_and_github_claim_flows -q -> 1 passed, 1 warning
  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest tests/test_wallet_api.py -q -> 33 passed, 1 warning
  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest -q -> 482 passed, 1 warning
  • uv run --extra dev python -m ruff check app/public_routes.py tests/test_wallet_api.py -> passed
  • uv run --extra dev python -m ruff format --check app/public_routes.py tests/test_wallet_api.py -> 2 files already formatted
  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m mypy app/public_routes.py -> success
  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python scripts/docs_smoke.py -> docs smoke ok
  • git diff --check -> clean

No private data, wallet material, tokens, signatures, live mutations, admin access, price/liquidity/exchange/bridge/off-ramp claims, or fabricated payout claims are included.

Summary by CodeRabbit

  • Bug Fixes
    • Enhanced wallet transaction type filtering with improved validation to reject inputs containing invalid characters, preventing processing errors and providing clearer feedback when invalid transaction types are submitted.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 29, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 1188ae30-48b4-420e-9802-9f1cc21e0fc8

📥 Commits

Reviewing files that changed from the base of the PR and between e00dc75 and cf16ede.

📒 Files selected for processing (2)
  • app/public_routes.py
  • tests/test_wallet_api.py

📝 Walkthrough

Walkthrough

The PR adds input validation to the wallet transaction type filter. The route now imports a control-character regex and checks the transaction_type parameter before querying ledger data, rejecting requests containing control characters with HTTP 400. Tests verify the validation rejects control-character values.

Changes

Transaction Type Validation

Layer / File(s) Summary
Transaction type control-character validation
app/public_routes.py, tests/test_wallet_api.py
Import CONTROL_CHAR_RE and add validation guard in wallet_page_context that rejects transaction_type values containing control characters with HTTP 400. Test coverage verifies the error response for control-character type filter values.

Possibly related PRs

  • ramimbo/mergework#521: Adds ledger filtering by transaction type in wallet_page_context; this PR adds the control-character validation guard to that same route parameter.
🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Title check ✅ Passed The title directly names the changed surface (wallet type filters) and describes the concrete change (rejecting control characters), matching the summary of changes.
Description check ✅ Passed The description provides summary, evidence of validation testing, and confirms no sensitive data is included, but omits the required Evidence section structure and Related bounty/issue format.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Mergework Public Artifact Hygiene ✅ Passed No investment, price, cash-out, payout, or private security claims detected. Changes are purely technical input validation for wallet type filters.
Bounty Pr Focus ✅ Passed PR diff matches Refs #576 scope: imports CONTROL_CHAR_RE, validates transaction_type in wallet_page_context(), adds control-char test with 400 assertions.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@barnacleagent-svg barnacleagent-svg left a comment

Choose a reason for hiding this comment

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

Verdict: APPROVED

Scope: Rejects control characters in wallet transaction type filter. Adds validation using existing CONTROL_CHAR_RE from ledger.service before processing the type parameter.

Checklist:

  • Diff: +5/-0 across 2 files
  • Uses existing CONTROL_CHAR_RE pattern (consistent with prior PRs)
  • Test verifies status_code 400 and correct error message
  • CI passing

Conclusion: Clean, focused input validation. Ready to merge.

Copy link
Copy Markdown

@eliasx45 eliasx45 left a comment

Choose a reason for hiding this comment

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

Reviewed current head cf16ede04aaf6506680949dbb754e95b5720af29.

Verdict: approve.

The change is small and matches the existing validation pattern: wallet_page_context() now rejects a non-null type filter containing control characters before trimming and passing it into the ledger transaction query. Clean unknown type filters still keep the existing empty-state behavior, while %09/tab style control input returns a bounded 400 response instead of being normalized into an ambiguous empty/all filter path.

Validation:

git diff --check origin/main...HEAD
# clean

git merge-tree --write-tree origin/main HEAD
# 4feadf80b877af5933fd3aaaefd1cd0e9aa5e0ef

C:\Users\home\Downloads\0-Elias\coprinter\mergework\.venv\Scripts\python.exe -m pytest tests/test_wallet_api.py -q
# 33 passed

C:\Users\home\Downloads\0-Elias\coprinter\mergework\.venv\Scripts\python.exe -m ruff check app/public_routes.py tests/test_wallet_api.py
# All checks passed!

C:\Users\home\Downloads\0-Elias\coprinter\mergework\.venv\Scripts\python.exe -m ruff format --check app/public_routes.py tests/test_wallet_api.py
# 2 files already formatted

C:\Users\home\Downloads\0-Elias\coprinter\mergework\.venv\Scripts\python.exe -m mypy app/public_routes.py
# Success: no issues found in 1 source file

C:\Users\home\Downloads\0-Elias\coprinter\mergework\.venv\Scripts\python.exe scripts/docs_smoke.py
# docs smoke ok

One environment note: the isolated worktree did not have its own .venv, so I ran the validation from the existing MergeWork checkout virtualenv against this PR worktree. No private data, wallet material, production mutations, tokens, signatures, price/liquidity/exchange/bridge/off-ramp claims, or fabricated payout claims were used.

Copy link
Copy Markdown

@wangedmund77-cmyk wangedmund77-cmyk left a comment

Choose a reason for hiding this comment

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

Reviewed current head cf16ede04aaf6506680949dbb754e95b5720af29 for bounty #578.

Evidence checked:

  • Inspected app/public_routes.py and tests/test_wallet_api.py.
  • git diff --check origin/main...origin/pr/609 returned clean.
  • Ran uv run pytest tests/test_wallet_api.py -q on the PR head: 33 passed, 1 StarletteDeprecationWarning.
  • GitHub mergeability is clean/MERGEABLE.

No blocker found. The new guard follows the existing public-query control-character validation pattern and the regression covers tab-only type input before rendering the wallet detail filter state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants