fix(files_sharing): enforce minimum search length in getUsers() and getGroups()

[DeepDiver1975]

Summary

• getUsers() and getGroups() in ShareesController only blocked empty strings, not strings shorter than the admin-configured user.search_min_length
• isSearchable() was already enforced in getRemote() but not in the other two search methods
• Fix adds the missing isSearchable() guard, closing the bypass

Security Impact

Low — authenticated-only endpoint; bypass only relevant when admin raises min length above default

Test plan

• New tests testGetUsersBlocksShortSearchTerm() and testGetGroupsBlocksShortSearchTerm() assert that backends are never queried when search term is below configured minimum
• Run make test TEST_PHP_SUITE=apps/files_sharing

🤖 Generated with Claude Code

[update-docs]

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

[DeepDiver1975]

Code Review — fix(files_sharing): enforce minimum search length in getUsers() and getGroups()

Overview: Adds the missing isSearchable() guard to getUsers() and getGroups() in ShareesController. The remote search path already had this guard; the user/group paths only blocked empty strings, allowing a single-character query to bypass the admin-configured minimum search length.

Correctness

The fix is minimal and consistent — four identical lines added symmetrically to both methods, mirroring the existing pattern in getRemote(). ✅

Test default in setUp(): The willReturn(true) default for isSearchable is the right call — it makes existing tests transparent to the new guard without requiring them all to be updated. New tests override this default. ✅

testGetUsersBlocksShortSearchTerm: Correctly asserts:

• isSearchable('a') returns false
• userManager->find() is never called
• result users array is empty
  ✅

testGetGroupsBlocksShortSearchTerm: Correctly asserts:

• isSearchable('a') returns false
• groupManager->search() is never called
• result groups array is empty
  ✅

One minor observation: Both new tests call $this->userSearch->expects($this->any())->method('getSearchMinLength'). isSearchable() is what the production code calls; getSearchMinLength isn't directly invoked by the patched code paths. This any() expectation is unreferenced and effectively dead — harmless but slightly confusing. Not blocking.

Changelog

The changelog entry in changelog/unreleased/41580 accurately describes the bypass and the fix. ✅

Summary

Aspect	Assessment
Fix	✅ Consistent, minimal — mirrors existing getRemote() pattern
Tests	✅ Both new cases assert backend is never queried for short terms
Test default	✅ setUp() default willReturn(true) doesn't break existing tests
Changelog	✅ Accurate and clear

Verdict: Ready to merge.

[dj4oC]

Adds a minimum-search-length guard (UserSearch::isSearchable()) to ShareesController::getUsers()/getGroups(), matching the guard already present on the remote-search path, to reduce user/group enumeration via short/empty search terms.

Never-auto-fix zone note: apps/files_sharing sharing-permission-adjacent logic — findings below are for human sign-off, not auto-merge, regardless of technical quality.

Findings

• CI is genuinely red right now (PHP Unit (8.3, sqlite) failing, other DB variants cancelled) — root-caused to a test-authoring bug, not the production logic: ShareesTest::setUp() registers a blanket, unconstrained isSearchable() stub (willReturn(true)) before the two new tests register a more specific ->with('a')->willReturn(false) stub. PHPUnit's matcher resolution returns the first eligible match, so the unconstrained setUp() stub wins and the short-circuit never actually triggers in the test, causing the expects($this->never())->method('find') assertion to fail. The underlying ShareesController guard placement itself is correct (verified: same pattern as the pre-existing remote-search guard, placed before the expensive backend call). Fix: give the two new tests their own dedicated mock rather than relying on setUp()'s stub being shadowed.
• Security finding worth fixing alongside this PR — CWE-1289/CWE-20 → CWE-204 enumeration bypass, OWASP A04:2021: the shared UserSearch::isSearchable() helper (which this PR extends to a new, more sensitive call site) uses \strlen(\trim($pattern)) — byte length, not character length. For multi-byte UTF-8 input, a single human-perceived character (e.g. one CJK ideograph = 3 bytes) can already satisfy user.search_min_length of 2, silently defeating the anti-enumeration control for large classes of locales. Recommend switching to mb_strlen().
• Also noted: there's no audit-logging subsystem anywhere in owncloud/core to record blocked-enumeration attempts — a repo-wide gap, not a regression from this PR, but relevant given the sensitivity of this code path.
• Performance: guard is correctly placed before the expensive userManager->find()/groupManager->search() calls — confirmed via the tests' expects($this->never()) assertions (once the mock-ordering bug above is fixed, these will actually verify what they claim to).

Verdict

Changes requested — fix the test's mock-ordering bug (CI-blocking) and switch UserSearch::isSearchable() to mb_strlen() (security gap in the helper this PR relies on more heavily). Requires human sign-off before merge per the never-auto-fix policy for this app.

🤖 Automated review by Claude Code (security · stability · performance · coverage)

---

Generated by Claude Code