Reject wallets page query filters#960
Conversation
|
Too much diff to scan? Review this PR in Change Stack to start with the highest-impact changes. 📝 WalkthroughWalkthroughThe PR adds query parameter validation to the ChangesQuery Parameter Validation for Wallets Endpoint
Possibly related PRs
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
TechEnthusGH
left a comment
There was a problem hiding this comment.
Reviewed current head 73c1715bf412fc1922c4a253b56be44014c83034 for Bounty #838.
Evidence checked:
- Diff is focused to
app/public_routes.pyandtests/test_wallet_api.py. /walletsstill accepts the supportedqsearch path, but now rejects API/list-style filters (limit,offset,status,account,repo,type) before rendering the HTML page. That matches the route-scope guard pattern and avoids silently ignoring unsupported filters on the wallets page.- Regression coverage extends
test_wallet_pages_reject_control_character_filtersto assert each unsupported wallet-list filter returns HTTP 400 with a specific message. - Mergeability/CI checked immediately before review: PR is open, non-draft,
mergeStateStatus=CLEAN,mergeable=MERGEABLE; GitHub checkQuality, readiness, docs, and image checksis successful; no human reviews or #838 claim mentions for PR #960 were present.
Local validation run from a clean checkout of the PR head:
git diff --check origin/main...HEAD-> passeduv run python -m pytest tests/test_wallet_api.py -q-> 45 passed, 1 warninguv run python -m ruff check app/public_routes.py tests/test_wallet_api.py-> passeduv run python -m ruff format --check app/public_routes.py tests/test_wallet_api.py-> passed
No blocker found.
Errordog2
left a comment
There was a problem hiding this comment.
Approved. I applied PR #960 at current head 73c1715bf412fc1922c4a253b56be44014c83034 onto current origin/main/base d7e9b530fffec7bd774da7708597648096a37393 with a clean 3-way apply.
Scope reviewed:
/walletsonly supports theqsearch filter, so rejectinglimit,offset,status,account,repo, andtypeavoids silently ignoring unsupported list-style filters./wallets/{address}?type=...remains unchanged and covered by the existing detail-page assertions.- The added test covers each unsupported wallet-list parameter and the expected 400 detail text.
Validation run locally:
python -m pytest tests/test_wallet_api.py -k "wallet_pages"-> 3 passedpython -m pytest tests/test_wallet_api.py-> 45 passedpython -m ruff check app/public_routes.py tests/test_wallet_api.py-> passedpython -m ruff format --check app/public_routes.py tests/test_wallet_api.py-> passedpython -m mypy app/public_routes.py-> passedgit diff --cached --check-> passed
This is narrowly scoped and safe to merge.
Fixes another
/walletspage query mismatch from bounty #798./claim #798
Report: #798 (comment)
Claim: #798 (comment)
What changed:
/walletsquery filters:limit,offset,status,account,repo,type.qwallet search behavior unchanged.Validation:
python3 -m pytest tests/test_wallet_api.py::test_wallet_pages_expose_transfer_and_github_claim_flows tests/test_wallet_api.py::test_wallet_pages_reject_control_character_filters -qpython3 -m pytest tests/test_wallet_api.py -qpython3 -m ruff check app/public_routes.py tests/test_wallet_api.pypython3 -m ruff format --check app/public_routes.py tests/test_wallet_api.pypython3 -m mypy app/public_routes.pygit diff --checkSummary by CodeRabbit
limit,offset,status,account,repo,type) with HTTP 400 error responses. Users attempting to use these parameters will receive clear error messages explaining that each parameter is not supported on the wallets page, improving API robustness.