microsoft / microsoft/simplechat

Search result cache can serve one user's results to another once file sharing is enabled

Open
#1,341 1 comment 0 reactions 1 assignee Claimed by @Bionic711 View on GitHub
bug
Dominant language
Python
Stars
152
Forks
116
Avg merge
7h 7m
Merged PRs (30d)
122

Description

**Severity: high, currently latent. Fix available.**

For group, public, and combined search scopes, `application/single_app/utils_cache.py`
deliberately omits `user_id` from the cache key so users can share entries. The
comment at line 391 states this: `# For group/public/all, exclude user_id to
enable cache sharing`.

Correctness therefore depends entirely on a fingerprint of reachable documents
standing in for user identity. That fingerprint query at `utils_cache.py:98-104`
is:

```sql
SELECT c.id, c.version
FROM c
WHERE c.user_id = @user_id OR ARRAY_CONTAINS(c.shared_user_ids, @user_id)
ORDER BY c.id
```

`ARRAY_CONTAINS` here is an exact match against a bare user id. But shares are
stored with a status suffix: `functions_documents.py:8818` appends
`f"{target_user_id},not_approved"`, and approved shares are stored as
`"{oid},approved"`. So the fingerprint never matches any shared document, and
shared documents never affect it.

Two users with different real document access can therefore compute an identical
fingerprint, collide on one cache key, and one can be served the other's search
results.

No live exposure exists while `enable_file_sharing` is disabled. It becomes
exploitable the moment that flag is turned on.

Compounding this: `enable_search_result_caching` defaults to `True` and is
exposed nowhere in the admin UI, so an operator cannot disable caching as a
mitigation without a code change and a redeploy. `search_cache_ttl_seconds` is
equally unreachable.

**Suggested fix.** Match the exact `,approved` value rather than the bare id. A
prefix match is not sufficient: `STARTSWITH(s, "{oid},")` matches `not_approved`
and `approved` identically, so a share moving from pending to approved would not
change the fingerprint even though it changes what the user can reach. The
fingerprint must also account for `enable_file_sharing` so it matches the search
authorization filter exactly rather than being a superset, since the property
required is that equal fingerprints imply equal access.

---

Found while enabling features on a fork of `v0.250.001`, verified against commit `ff8059163e09ede433003b1ed2822061c41239fe`. Line numbers are against that baseline.

We have a working fix and a functional test for this on our fork. Happy to open a PR if that is useful, or to share the patch and let you take a different approach. No expectation either way.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.