Skip to content

Reject oversized account filters#863

Open
NiXouuuu wants to merge 2 commits into
ramimbo:mainfrom
NiXouuuu:codex/account-length-799
Open

Reject oversized account filters#863
NiXouuuu wants to merge 2 commits into
ramimbo:mainfrom
NiXouuuu:codex/account-length-799

Conversation

@NiXouuuu
Copy link
Copy Markdown

@NiXouuuu NiXouuuu commented Jun 4, 2026

Summary

  • Reject normalized account values longer than 128 characters before public account/activity filtering.
  • Preserve the existing stricter validators for github:, mrwk1, reserve:bounty:, and treasury:mrwk account forms.
  • Add regression coverage for oversized generic accounts across /api/v1/activity, account API/page routes, and MCP balance calls.

Bounty #799
Source report: #798 (comment)

Validation

  • uv run --extra dev pytest tests/test_activity.py::test_activity_query_rejects_control_characters tests/test_account_validation.py::test_account_views_reject_oversized_generic_accounts -q -> 2 passed, 1 existing Starlette/httpx warning.
  • uv run --extra dev pytest tests/test_activity.py tests/test_account_validation.py -q -> 52 passed, 1 existing Starlette/httpx warning.
  • uv run --extra dev ruff check app/accounts.py tests/test_activity.py tests/test_account_validation.py -> passed.
  • uv run --extra dev ruff format --check app/accounts.py tests/test_activity.py tests/test_account_validation.py -> 3 files already formatted.
  • uv run --extra dev mypy app/accounts.py app/activity.py -> success.
  • uv run --extra dev python scripts/docs_smoke.py -> docs smoke ok.
  • git diff --check origin/main...HEAD -> clean.
  • git merge-tree --write-tree origin/main HEAD -> clean tree abda6f4c7b8cc6d19c1ef4bc0fd2711b121cfce1.

Scope

Public account normalization and public activity/account inspection only. No payout execution, treasury mutation, wallet mutation, transfer signing, ledger mutation, admin-token behavior, private data, secrets, bridge/exchange/cash-out behavior, or MRWK price behavior is changed.

Summary by CodeRabbit

  • Bug Fixes

    • Account inputs are now limited to 128 characters; overly long accounts are rejected with HTTP 400 and the message "account is too long" across API endpoints and query parameters.
  • Tests

    • Test suite expanded to cover oversized account rejection across web pages, API endpoints, and internal RPC calls, including prefixed/parameterized cases to preserve existing validation details.

Copy link
Copy Markdown
Contributor

@xiefuzheng713-alt xiefuzheng713-alt left a comment

Choose a reason for hiding this comment

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

Approved current head 8a3d39b as non-author.

Scope checked:

  • app/accounts.py: added ACCOUNT_MAX_LENGTH=128, normalized_account() now returns 400 "account is too long" for generic account inputs before lowercasing.
  • tests/test_account_validation.py: added oversized-account assertions across /api/v1/accounts, /accounts, and MCP get_balance.
  • tests/test_activity.py: added /api/v1/activity oversized account rejection assertion.

Local checks on this head:

  • uv run --extra dev pytest tests/test_activity.py tests/test_account_validation.py -q -> 51 passed, 1 existing Starlette/httpx warning.
  • uv run --extra dev ruff check app/accounts.py tests/test_activity.py tests/test_account_validation.py -> passed.
  • uv run --extra dev ruff format --check app/accounts.py tests/test_activity.py tests/test_account_validation.py -> 3 files already formatted.
  • uv run --extra dev mypy app/accounts.py app/activity.py -> success.

No actionable issues found.

/claim #799

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 4, 2026

Review Change Stack

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: e1f1d25f-10c6-4270-88e8-eec6ffa4b858

📥 Commits

Reviewing files that changed from the base of the PR and between 8a3d39b and 778bf33.

📒 Files selected for processing (2)
  • app/accounts.py
  • tests/test_account_validation.py

📝 Walkthrough

Walkthrough

Adds ACCOUNT_MAX_LENGTH = 128 and an early trimmed-length check in normalized_account to raise HTTP 400 ("account is too long") for >128-character inputs; tests added/updated to verify rejection across API, page, MCP, and activity query paths.

Changes

Account Length Validation

Layer / File(s) Summary
Account length validation contract and implementation
app/accounts.py
Module constant ACCOUNT_MAX_LENGTH = 128 added; normalized_account performs an early check on the trimmed account length and raises HTTPException(status_code=400, detail="account is too long") if it exceeds 128 characters.
Validation test coverage
tests/test_account_validation.py, tests/test_activity.py
Added test_account_views_reject_oversized_generic_accounts and a parametrized test_account_views_preserve_prefixed_validation_for_oversized_accounts asserting API/page return HTTP 400 with expected details for oversized inputs while MCP /mcp returns an invalid-tool-arguments error; test_activity_query_rejects_control_characters gains a case asserting /api/v1/activity returns HTTP 400 with detail "account is too long" for a 129-character account parameter.

Possibly related PRs

  • ramimbo/mergework#750: Changes to normalized_account validation logic affecting oversized-account and control-character checks.
  • ramimbo/mergework#821: Related updates routing activity account validation through normalized_account and overlapping activity API behavior.
🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Reject oversized account filters' directly names the changed surface and clearly describes the main change: enforcing a 128-character limit on account values.
Description check ✅ Passed The description covers all required template sections: summary of changes, validation evidence (all test checks passed), scope clarification, and related bounty reference; a complete and detailed submission.
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, or payout claims found. MRWK described correctly; scope note disclaims cash-out/price changes.
Bounty Pr Focus ✅ Passed PR matches stated files and scope: app/accounts.py adds 128-char limit after prefixed validators; tests verify generic and prefixed account rejection; changes limited to public account inspection.

✏️ 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

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 452081d2-296a-4794-b92a-ee5f5e32157a

📥 Commits

Reviewing files that changed from the base of the PR and between d4d0e48 and 8a3d39b.

📒 Files selected for processing (3)
  • app/accounts.py
  • tests/test_account_validation.py
  • tests/test_activity.py

Comment thread app/accounts.py Outdated
Comment thread tests/test_account_validation.py
Copy link
Copy Markdown
Contributor

@xiefuzheng713-alt xiefuzheng713-alt left a comment

Choose a reason for hiding this comment

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

I reviewed the current head for the oversized account validation fix.

Checked changed paths:

  • app/accounts.py
  • tests/test_account_validation.py
  • tests/test_activity.py

Focus review:

  • Generic oversized accounts now return 400 �ccount is too long in
    ormalized_account().
  • Prefix-specific validators (github:, mrwk1,
    eserve:bounty:, reasury:) keep their existing stricter errors because they are matched before the generic length gate.
  • Regression coverage added for API, page, MCP surfaces and activity query boundary.

I did not see regressions in the touched behavior. mergeStateStatus is currently UNSTABLE because CI/quality checks are still in progress.

/claim #799

This looks aligned with issue #798 report #4621709113 and the current bounty scope.

Copy link
Copy Markdown

@tudorian95 tudorian95 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 778bf3306cf51d2e4f505ac232060575dda6cbfc for Bounty #838.

What I checked:

  • Inspected the focused diff in app/accounts.py, tests/test_account_validation.py, and tests/test_activity.py.
  • Confirmed the live production before-state is still reproducible with harmless GETs: a 129-character generic account returns HTTP 200 from both GET /api/v1/accounts/{account} and GET /api/v1/activity?account={account}.
  • In Docker/uv on a fresh checkout of this PR head:
    • uv run --extra dev pytest tests/test_activity.py::test_activity_query_rejects_control_characters tests/test_account_validation.py::test_account_views_reject_oversized_generic_accounts -q -> 2 passed, 1 existing Starlette/httpx warning.
    • uv run --extra dev pytest tests/test_activity.py tests/test_account_validation.py -q -> 56 passed, 1 existing warning.
    • uv run --extra dev ruff check app/accounts.py tests/test_activity.py tests/test_account_validation.py -> passed.
    • uv run --extra dev ruff format --check app/accounts.py tests/test_activity.py tests/test_account_validation.py -> passed.
    • uv run --extra dev mypy app/accounts.py app/activity.py -> success.
    • uv run --extra dev python scripts/docs_smoke.py -> docs smoke ok.
    • git diff --check origin/main...HEAD -> clean.
  • git merge-tree --write-tree origin/main HEAD succeeded with tree 48adf907a11e796e11f6b09911016cb7f4461f9a.
  • GitHub reports the current head as MERGEABLE.

The change adds a generic account length cap while intentionally preserving the more specific prefixed account validators for GitHub, MRWK wallet, reserve, and treasury forms. I did not find blockers.

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.

3 participants