fix(budgets): escape the member search filter before it reaches ILIKE

[aivong-openhands]

HUMAN:

• A human has tested these changes.

AGENT:

---

Why

A metacharacter typed into the members search box widened the filter instead of narrowing it. Both OrgMemberStore.get_org_members_count and OrgMemberStore.get_org_members_paginated interpolated email_filter straight into User.email.ilike(f'%{email_filter}%'), so searching for % matched every member of the organization and _ matched any address at all. The members page (frontend/src/routes/manage-organization-members.tsx) feeds the same debounced term to both, so both have to be fixed together — escaping only the listing swaps one wrong answer for an empty table beside a total of 47.

The budget page already escaped its own search term through a private _escape_ilike in org_budget_service.py. Rather than copy it a third time, this hoists it to utils/sql.escape_ilike and has all three call sites — budget search, member listing, member count — share one copy.

Summary

• Add utils/sql.escape_ilike, moved from server/services/org_budget_service.py, and use it from the budget search.
• Escape the email filter in both get_org_members_count and get_org_members_paginated and pass an explicit escape='\'.
• Un-skip the reproduction test that searches for a bare % against a seeded organization.
• Add test_member_email_filter_treats_metacharacters_literally, parametrised over %, _, \, %_\, a literal term and a differently-cased literal term, asserting the count and listing paths agree on real rows in a real database.
• Add test_budget_user_search_treats_metacharacters_literally against _build_user_budget_rows, so the third caller of the shared helper is covered too.

Issue Number

N/A

How to Test

.venv/bin/python -m pytest -q \
  tests/unit/server/services/test_org_member_financial_service.py \
  tests/unit/test_org_member_store.py \
  tests/unit/test_org_budget_service.py

Expect 97 passed, 9 skipped.

Video/Screenshots

No UI change, so the evidence is the pair of numbers the members page renders side by side. Seeding an org with 47 members (user0@example.com … user46@example.com) and calling the count and listing stores with the same search term, as the page does:

On main — the bug. Searching for % returns the whole org instead of nothing:

search='%'      count endpoint=47  listing rows=47
search='user1'  count endpoint=11  listing rows=11

At the previous head of this PR — the gap the review caught. Only the listing was escaped, so the page would have shown an empty table next to a total of 47:

search='%'      count endpoint=47  listing rows=0
search='user1'  count endpoint=11  listing rows=11

With this push. Both endpoints agree, and literal searches are unaffected:

search='%'      count endpoint=0   listing rows=0
search='user1'  count endpoint=11  listing rows=11

Mutation checks on the new tests: reverting either .ilike() call to the unescaped form fails 3 of the parametrised cases, and reordering the .replace() chain in escape_ilike so the backslash is handled last fails 2 of them. The ordering is therefore pinned by a test, not just described in a comment.

A mutation review on this PR found two further live mutants, both now killed. Dropping the escape_ilike call from the budget search (escaped = search_value) left the suite green, because the helper this PR hoists gained a third caller with no coverage; the new budget test fails 3 of its 4 cases under that mutant. Making get_org_members_count case-sensitive while the listing stayed on ilike also survived — the same count/listing divergence this PR closes, reached from the other side — and the added ALICE case is the only failure under it.

Type

• Bug fix
• Feature
• Refactor
• Breaking change
• Docs / chore

Notes

One of a set of draft PRs, each carrying a single defect the Quint model for org budgets surfaced, together with the reproduction test that was already committed but skipped.

To be explicit, since "escaping" and "SQL" appear together: this is not an injection fix. SQLAlchemy parameterises the pattern, so the term never escapes the string literal. Nor is the explicit escape='\\' load-bearing: it is already Postgres's default for LIKE/ILIKE, so it documents intent at the call site rather than changing behaviour. The defect is wildcard semantics — an admin searching for one member was handed the whole roster.

Not touched here: server/services/org_conversation_service.py:291 builds an unescaped ILIKE pattern the same way for the conversation search. It is the same class of bug but a different page, so it belongs in its own PR rather than widening this one.

🤖 Generated with Claude Code

---

Enterprise server image for this PR:

ghcr.io/openhands/enterprise-server:sha-a9e7ec8

[github-actions]

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
server/services
org_budget_service.py
storage
org_member_store.py
utils
sql.py
Project Total

This report was generated by python-coverage-comment-action

[aivong-openhands]

Note: submitted as a COMMENT review rather than REQUEST_CHANGES because GitHub does not allow requesting changes on a pull request authored by the same account. The verdict below is nonetheless needs rework — please treat the CRITICAL ISSUES as blocking.

Code Review

🟡 Acceptable — the escaping itself is correct, minimal, and matches the existing _escape_ilike helper's behaviour exactly. The problem is that it was applied to one of the two functions that need it, and it was inlined instead of reusing the helper that already exists for this.

[CRITICAL ISSUES]

• [storage/org_member_store.py, L216] Same Defect, Untouched, Twelve Lines Up: get_org_members_count — in the same class, in the same file, 40 lines above the fixed line — still does User.email.ilike(f'%{email_filter}%') with no escaping. It is not a dead path: server/routes/orgs.py:1054 exposes it as GET /orgs/{org_id}/members/count, and frontend/src/routes/manage-organization-members.tsx:54-62 calls useOrganizationMembers and useOrganizationMembersCount with the same debouncedEmailFilter.

  So after this PR, typing % into the members search box returns zero rows from the fixed listing endpoint while the unfixed count endpoint reports the whole organization. The user-visible result is a table showing "no results" next to a total of 47. The PR trades one wrong answer for an internally contradictory pair, which is arguably harder to diagnose. Both call sites must be fixed together.

[IMPROVEMENT OPPORTUNITIES]

• [storage/org_member_store.py, L252-257] Duplicated Logic: _escape_ilike already exists at server/services/org_budget_service.py:382 and is character-for-character what this inlines. The PR description even cites it as prior art — then reimplements it instead of calling it. Escaping rules that exist in two places drift; the next person to handle [ in a LIKE pattern will fix one. Hoist _escape_ilike somewhere shared (utils/, or storage/) and have all three call sites — budget search, member listing, member count — use it.

• [storage/org_member_store.py, L250-251] Comment States the Requirement, Not the Reason: "A metacharacter typed into the members search box must match itself rather than widen the filter to the org" restates what the .replace() chain plainly does. The non-obvious part worth recording is the escape='\\' argument — that SQLAlchemy needs it explicitly or the backslashes are passed through as literal characters rather than escapes. Everything else is inferable from the code.

[TESTING GAPS]

• No Test for get_org_members_count: tests/unit/test_org_member_store.py:838 has test_get_org_members_count_with_email_filter, but it only exercises a literal term. Nothing pins the metacharacter behaviour on the count path, which is precisely why the gap above is invisible to CI — all checks are green on a PR that leaves the two endpoints disagreeing.

• Credit where due on the listing test: test_member_search_never_matches_beyond_the_literal_term is a genuinely good test. It seeds three real members against a real session, runs the real query, and asserts on the result set rather than on a mock call. It fails on the old code and passes on the new. That is exactly the right shape — please give the count path the same treatment.

• Coverage of the escape set is thin: the test searches for % only. _ (matches any single character) and \ (the escape character itself, and the reason for the .replace('\\', '\\\\') ordering) are handled by the code but unpinned by any test. The backslash case in particular is where a naive reordering of those three .replace() calls silently breaks, and nothing would catch it. Parametrising over ['%', '_', '\\', '%_\\'] costs one decorator.

• [PR description] Evidence: How to Test lists only pytest invocations. Per this repo's bar, test output alone is not evidence. Since the described symptom is user-facing and trivially reproducible — type % into the members search — a screenshot of the members page before and after, or a curl against GET /orgs/{org_id}/members/financial?email=%25 with the response body, would settle it. That would also have surfaced the count/listing disagreement immediately, since the UI renders both numbers side by side. Please also include the agent conversation URL, given the description notes this was agent-generated.

[SECURITY NOTE]

For the record, since "escaping" and "SQL" appear together: this was never an injection risk. SQLAlchemy parameterises the pattern, so the term cannot escape the string literal — the bug is a wildcard-semantics bug confined to matching behaviour. Worth stating plainly so nobody back-ports this as a security fix or reaches for the wrong severity. The real consequence is an information-disclosure-shaped one: an admin searching for a specific member got handed the entire org roster, which is a correctness and UX failure rather than a boundary failure.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟢 LOW

Eight lines in one query builder, no schema change, no API contract change, no dependency change, all CI green. The escaping is provably correct and the behaviour change is strictly narrowing — any search term that worked literally before still works identically, and only terms containing %, _, or \ change meaning, which is the entire point. Worst case if the fix is wrong is a search that under-matches, which is visible and harmless. The count-endpoint gap raises the confusion cost but not the risk: it is already broken on main, and this PR does not make it worse in isolation, only inconsistent with its neighbour.

VERDICT:

❌ Needs rework — the fix is right, the scope is not. Ship the count path with it.

KEY INSIGHT:

Two functions in the same class build the same ILIKE pattern from the same user input and are called with the same term by the same page; fixing one and not the other converts a consistently wrong answer into two mutually contradictory ones.

---

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

---

This review was generated by an AI agent (OpenHands) on behalf of the requesting user.

[aivong-openhands]

Review addressed in 34a1e0d. Taking the points in order:

Critical — get_org_members_count untouched. Correct, and the resulting failure mode was worse than the bug. Both queries now escape through the same helper. The PR description carries the evidence: against a seeded 47-member org, main returns 47/47 for a % search, the previous head returned 47 from the count and 0 from the listing, and this push returns 0/0. Literal searches are unchanged at 11/11.

Duplicated logic. _escape_ilike moved out of org_budget_service.py into utils/sql.escape_ilike. All three call sites — budget search, member listing, member count — now share it.

Comment states the requirement, not the reason. The call site is back to # Apply email filter if provided. The two non-obvious facts — the backslash replacement must come first, and escape='\' is required or the escaping is a no-op — are in the helper's docstring, next to the code they constrain.

Testing gaps. test_member_email_filter_treats_metacharacters_literally is parametrised over %, _, \, %_\ and a literal term, and asserts the count and listing paths return the same set against a real database. Mutation checks: reverting either .ilike() to the unescaped form fails 3 of 5 cases; reordering the .replace() chain so the backslash is last fails 2 of 5. The ordering and both call sites are pinned, so the endpoints cannot drift apart silently again.

Evidence. The description now shows the count/listing pair in all three states rather than only a pytest summary line.

Security note. Agreed and now stated in the description: SQLAlchemy parameterises the pattern, so this was never injection. It is wildcard semantics.

One thing I deliberately did not do: server/services/org_conversation_service.py:291 has the same unescaped pattern for the conversation search. Same class of bug, different page, and pulling it in would widen a PR that is meant to carry one defect — noted in the description for a follow-up instead.

92 passed, 9 skipped across the member store, financial service and budget service suites; pre-commit clean.

---

This comment was created by an AI agent (OpenHands) on behalf of the requesting user.

[aivong-openhands]

Mutation review of the tests in this PR

I hand-wrote 14 mutants against the lines this PR touches and ran them over the
three files listed in How to Test. Baseline on 34a1e0d: 92 passed,
9 skipped in 12.7s.

Controls — the PR's central claims are pinned

Mutant	Result
C1 revert get_org_members_count to the unescaped ilike(f'%{email_filter}%')	❌ caught
C2 revert get_org_members_paginated to the unescaped form	❌ caught
C3 reorder escape_ilike so the backslash is handled last	❌ caught

All three die, so the claims in the description hold up. What makes them die is
specific: test_member_email_filter_treats_metacharacters_literally asserts on
{member.user.email for member in members} — the whole set, not a length — and
asserts count == len(expected) in the same test body against the same seeded
rows. That pairing is what catches a one-sided fix; escaping only the listing
leaves count at 3 while the set is empty, and the test fails on the count
line. Seeding a literal dave\ops@example.com rather than asserting on a
generated pattern string is what makes C3 land, and running against real
Postgres rather than a mocked session is what makes any of it meaningful.

Survivors

Mutant	Result
M1 budget search stops escaping (escaped = search_value)	✅ 92 passed
M8 get_org_members_count becomes case-sensitive (ilike → like)	✅ 92 passed
M2 / M3 drop escape='\' from the count / listing ilike	✅ 92 passed
M7 drop escape='\' from both budget ilike columns	✅ 92 passed
M6 paginated guard if email_filter: → if email_filter is not None:	✅ 92 passed

---

M1 — the third call site is the one nothing tests

This PR hoists _escape_ilike out of org_budget_service.py into
utils/sql.escape_ilike and keeps the budget search calling it. Deleting that
call entirely — escaped = search_value — leaves the suite green.

That is the regression risk the refactor introduces. The helper now has three
callers and only two of them are held down; someone tidying utils/sql.py, or
inlining it back, gets no signal from the budget page. The bug is identical to
the one this PR fixes: an admin searching % on the budgets page is handed
every member of the org with their spend attached.

_build_user_budget_rows is reachable directly, and budget_org already
exists as a fixture in tests/unit/test_org_budget_service.py:

@pytest.mark.asyncio
@pytest.mark.parametrize(
    'users_search,expected',
    [
        ('%', set()),
        ('_', {'bob_smith@example.com'}),
        ('\\', {'dave\\ops@example.com'}),
        ('alice', {'alice@example.com'}),
    ],
)
async def test_budget_user_search_treats_metacharacters_literally(
    async_session_maker, budget_org, users_search, expected
):
    """The budget page search box must narrow the roster, not widen it."""
    emails = ['alice@example.com', 'bob_smith@example.com', 'dave\\ops@example.com']
    async with async_session_maker() as session:
        role = Role(name='member', rank=1)
        session.add(role)
        await session.flush()
        for i, email in enumerate(emails):
            user_id = uuid4()
            session.add_all(
                [
                    User(id=user_id, current_org_id=budget_org.id, email=email),
                    OrgMember(
                        org_id=budget_org.id,
                        user_id=user_id,
                        role_id=role.id,
                        llm_api_key=f'test-key-{i}',
                        status='active',
                    ),
                ]
            )
        session.add(OrgBudgetSettings(org_id=budget_org.id, enabled=True))
        await session.commit()

    async with async_session_maker() as session:
        settings = (
            await session.execute(
                select(OrgBudgetSettings).where(
                    OrgBudgetSettings.org_id == budget_org.id
                )
            )
        ).scalar_one()
        rows, total = await OrgBudgetService(session)._build_user_budget_rows(
            budget_org.id,
            settings,
            None,
            users_page=1,
            users_per_page=50,
            users_search=users_search,
            users_status=None,
        )

    assert {row['user_email'] for row in rows} == expected
    assert total == len(expected)

Verified both ways: 4 passed on this branch unmodified, and M1 goes from
SURVIVED to killed with it applied. (Note the row key is user_email, not
email.)

M8 — the count path's case-insensitivity is unasserted

Changing ilike → like in get_org_members_count only, leaving the listing
on ilike, keeps the suite green. test_get_org_members_paginated_email_filter_case_insensitive
covers the listing; there is no counterpart for the count.

This is the same count/listing divergence the PR was written to close, reached
from the other direction: an admin typing Alice would see one row in the
table under a total of 0. The escaping work touched exactly this expression,
so it is worth pinning now.

One more parametrise case on the test you already added covers it, since that
test asserts both paths:

        ('alice', {'alice@example.com'}),
        ('ALICE', {'alice@example.com'}),

Verified both ways: 6 passed on this branch unmodified, M8 killed with it
applied, and C1 still dies.

M2 / M3 / M7 — I think these are equivalent mutants

Dropping escape='\' from any of the three ilike() calls leaves the suite
green. I do not think that is a test gap. ESCAPE '\' is already Postgres's
default for LIKE/ILIKE, so the argument compiles to a no-op here:

with escape -> email ILIKE '%a\_b%' ESCAPE '\'
no escape   -> email ILIKE '%a\_b%'

and both forms agree on real rows ('bob_smith' ILIKE '%\_%' is true with and
without it; 'alice' is false with and without). No test can distinguish them
while Postgres is the only backend, which makes these unkillable rather than
unasserted.

Worth flagging only because the escape_ilike docstring states the opposite —
"without it the backslashes reach the database as literal characters and the
escaping does nothing". On Postgres that is not so. Being explicit is
defensible as documentation of intent, but the docstring currently reads as a
correctness requirement a reader cannot verify.

M6 — defended outside the code under test

Relaxing the listing's if email_filter: to if email_filter is not None:
survives, which would make an empty-string filter apply ILIKE '%%' in the
listing while the count kept its truthiness guard. Since User.email is
nullable, that drops null-email members from the table but not from the total —
the divergence again.

It cannot be reached through the API: all three routes in server/routes/orgs.py
declare email with min_length=1, so ?email= is rejected at validation. A
store-level test would pin it, but the route constraint is the stronger
guarantee. Noting it as a reason the guard should stay, not as a missing test.

Not a test gap

• server/services/org_conversation_service.py:291 still builds
  f'%{search}%' unescaped for the conversation search, as the description
  says. Confirmed it is untouched on this branch.
• get_org_members_count only joins User when a filter is present, while
  get_org_members_paginated always inner-joins it. FK integrity makes these
  agree today, but it is the kind of asymmetry that produced the bug this PR
  fixes.

---

This comment was generated by an AI assistant on behalf of the user.

[aivong-openhands]

Thanks — the mutation review was useful, and both survivors you flagged were real gaps. Addressed in a9e7ec8.

M1 — budget search untested. You were right that this was the regression risk the refactor introduced: hoisting _escape_ilike into utils/sql.escape_ilike gave the helper a third caller with nothing holding it down. Added test_budget_user_search_treats_metacharacters_literally against _build_user_budget_rows, essentially as you wrote it (thanks for the user_email note, and for spotting that budget_org was already there to reuse). Verified: 4 passed unmodified; with escaped = search_value applied, 3 of the 4 cases fail.

M8 — count path case-insensitivity. Added ('ALICE', {'alice@example.com'}) to the store test. Verified: 6 passed unmodified; with ilike changed to like in get_org_members_count only, the ALICE case fails and nothing else does, which is exactly the count/listing split the test exists to catch.

M2 / M3 / M7 — agreed, equivalent. ESCAPE '\' is already the Postgres default, so no test can distinguish those forms. The useful part of your comment was that the docstring claimed otherwise — it read as a correctness requirement a reader could not verify. Rewrote it to say the argument documents intent at the call site rather than changing behaviour, and kept the argument.

M6 — leaving as is. Your reasoning holds: the min_length=1 constraint on all three routes in server/routes/orgs.py is the stronger guarantee, and a store-level test would pin a state the API cannot reach. Worth having in the thread as the reason the truthiness guard should stay.

Full run after the change: 97 passed, 9 skipped (was 92/9, i.e. the five new cases). C1, C2 and C3 all still die.

On the two "not a test gap" notes: org_conversation_service.py:291 stays untouched here deliberately — same bug class, different page, its own PR. The count/listing join asymmetry you noticed is a fair observation; I left it alone to keep this diff to the defect.

---

This comment was written by an AI agent (OpenHands).