Reject URL-encoded special characters in account path IDs (#1083)#1169
Reject URL-encoded special characters in account path IDs (#1083)#1169yanyishuai wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
Account identifier validation hardening
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ebeb3e7e-be49-4617-91cf-235031bc3a18
📒 Files selected for processing (3)
app/accounts.pydocs/agent-guide.mdtests/test_account_validation.py
| URL-encoded special characters (for example `%21%40%23%24`) and other malformed | ||
| path IDs return HTTP 400 with `{"detail":"account identifier is malformed"}` on | ||
| both `/api/v1/accounts/{account}` and `/accounts/{account}`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Narrow the generic 400 detail to malformed plain identifiers.
Line 173 currently reads as if all malformed account path IDs return {"detail":"account identifier is malformed"}, but normalized_account() still emits prefix-specific 400 details for invalid github:, reserve:, and treasury: identifiers. Reword this to cover malformed plain legacy names or percent-decoded special characters only, so the guide matches the actual API contract. As per path instructions, "Review docs for short, concrete prose" and coding guidelines, "Update examples when API, label, or ledger behavior changes."
Suggested wording
- URL-encoded special characters (for example `%21%40%23%24`) and other malformed
- path IDs return HTTP 400 with `{"detail":"account identifier is malformed"}` on
- both `/api/v1/accounts/{account}` and `/accounts/{account}`.
+ URL-encoded special characters (for example `%21%40%23%24`) and malformed plain
+ legacy names return HTTP 400 with `{"detail":"account identifier is malformed"}`
+ on both `/api/v1/accounts/{account}` and `/accounts/{account}`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| URL-encoded special characters (for example `%21%40%23%24`) and other malformed | |
| path IDs return HTTP 400 with `{"detail":"account identifier is malformed"}` on | |
| both `/api/v1/accounts/{account}` and `/accounts/{account}`. | |
| URL-encoded special characters (for example `%21%40%23%24`) and malformed plain | |
| legacy names return HTTP 400 with `{"detail":"account identifier is malformed"}` | |
| on both `/api/v1/accounts/{account}` and `/accounts/{account}`. |
Sources: Coding guidelines, Path instructions
| def test_account_views_accept_plain_alphanumeric_account(sqlite_url: str) -> None: | ||
| client = _setup_app(sqlite_url) | ||
|
|
||
| resp = client.get("/api/v1/accounts/plain-account") | ||
|
|
||
| assert resp.status_code == 200 | ||
| assert resp.json()["account"] == "plain-account" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add one positive case for . or _ in legacy names.
Line 362 only proves hyphenated names, but the new contract and docs allow plain legacy identifiers with . and _ too. Add at least one accepted case such as plain.name_1 so this test covers the full advertised character set. As per path instructions, "Focus on whether tests prove the changed behavior and include negative, replay, boundary, or regression cases where relevant," and coding guidelines, "Add or update tests for changed behavior."
Suggested test shape
- def test_account_views_accept_plain_alphanumeric_account(sqlite_url: str) -> None:
+ `@pytest.mark.parametrize`("account", ["plain-account", "plain.name_1"])
+ def test_account_views_accept_plain_legacy_account(sqlite_url: str, account: str) -> None:
client = _setup_app(sqlite_url)
- resp = client.get("/api/v1/accounts/plain-account")
+ resp = client.get(f"/api/v1/accounts/{account}")
assert resp.status_code == 200
- assert resp.json()["account"] == "plain-account"
+ assert resp.json()["account"] == account📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_account_views_accept_plain_alphanumeric_account(sqlite_url: str) -> None: | |
| client = _setup_app(sqlite_url) | |
| resp = client.get("/api/v1/accounts/plain-account") | |
| assert resp.status_code == 200 | |
| assert resp.json()["account"] == "plain-account" | |
| `@pytest.mark.parametrize`("account", ["plain-account", "plain.name_1"]) | |
| def test_account_views_accept_plain_legacy_account(sqlite_url: str, account: str) -> None: | |
| client = _setup_app(sqlite_url) | |
| resp = client.get(f"/api/v1/accounts/{account}") | |
| assert resp.status_code == 200 | |
| assert resp.json()["account"] == account |
Sources: Coding guidelines, Path instructions
qingfeng312
left a comment
qingfeng312
left a comment
There was a problem hiding this comment.
I found one blocking issue before this is merge-ready.
The validation change itself is focused and the route coverage is useful: normalized_account() now rejects malformed plain account identifiers, while the existing github:, mrwk1, reserve:bounty:, and treasury:mrwk paths remain routed through their stricter branches.
Blocking issue:
CI is failing on formatting. The current head a0066c3b5c7c91d8c0bf2c7ff2fff9c708ec990d passes the full pytest run (910 passed, 1 warning), but ruff format --check . reports:
Would reformat: tests/test_account_validation.py
1 file would be reformatted, 125 files already formatted
Please run the formatter on tests/test_account_validation.py and push the resulting diff. After that, this should be a small re-check because the functional tests are already green.
Validation:
- Inspected PR #1169 at head
a0066c3b5c7c91d8c0bf2c7ff2fff9c708ec990d. - Reviewed changed files:
app/accounts.py,docs/agent-guide.md,tests/test_account_validation.py. - Checked the failed CI log for run
28324649863. - Confirmed the failure is formatter-only, not a test failure.
Refs #1009
yanyishuai
force-pushed
the
bounty-1083-account-path-special-chars
branch
from
a0066c3 to
12b63fd
Compare
yanyishuai
force-pushed
the
bounty-1083-account-path-special-chars
branch
from
12b63fd to
d412c45
Compare
qingfeng312
left a comment
qingfeng312
left a comment
There was a problem hiding this comment.
Reviewed current head d412c454e4ec71a362e3470abb1759d144d228d8.
Approved. The current head adds a focused account-identifier guard for decoded special characters and covers both API and HTML account routes with regression tests. Plain alphanumeric account identifiers remain accepted, and the hosted quality check is green. I do not see a remaining blocker in this diff.
2 participants
Related to #1083
Summary
Reject URL-encoded special characters and other malformed plain account path identifiers on public account routes. Ledger, wallet, and bounty paths already fail closed; this aligns account lookups with the same behavior.
Changes
normalized_account()returns HTTP 400 for identifiers outside canonical shapes (github:,mrwk1,reserve:bounty:,treasury:mrwk, or plain[A-Za-z0-9._-]+legacy names)%21%40%23%24on JSON API and HTML routesEvidence
Wallet: Do4v7foHJvRJLpRRoGaVPWX6DDEjX3yTK7J91gpwUQpE
Summary by CodeRabbit
Bug Fixes
Documentation
Tests