search

element-hq / element-web

A glossy Matrix collaboration client for the web.
https://element.io
GNU Affero General Public License v3.0
11.23k stars 2k forks source link

Search relies on optional `count` field in response #27517

Closed Benjamin-L closed 5 months ago

Benjamin-L commented 5 months ago

Steps to reproduce

  1. Spin up a server that does not return a value for the optional count field in the response body for the /client/v3/search endpoint.
  2. Use the search function in a non-E2EE room

For a server implementation reproducing the element bug, you can use commit 5c39c7c5 of grapevine, conduit df16b2ba (before a workaround for element was added in edfd3c1f), or conduwuit a2ee6b41 with the element-hacks feature disabled (before the workaround was made unconditional in 0214caea).

Outcome

What did you expect?

Either some search results appear or it shows that there were no results. The count field is marked as optional in the spec, so it's absence should not break search client-side.

What happened instead?

Hangs indefinitely, showing the loading animation:

image

Returning any value in the count field causes search to function as expected.

Operating system

NixOS (linux)

Browser information

Firefox 126.0

URL for webapp

app.element.io

Application version

Element version: 1.11.67 Crypto version: Rust SDK 0.7.0 (61b175b), Vodozemac 0.5.1

Homeserver

Grapevine 5c39c7c5fff0e1a59999f0c13bf4aa48cb159129

Will you send logs?

Yes

t3chguy commented 5 months ago

Will you send logs? Yes

Not seeing any logs from you

Benjamin-L commented 5 months ago

Will you send logs? Yes

Not seeing any logs from you

Ah, sorry, I forgot to do that yesterday. I've uploaded logs now.

Thanks for putting together a fix so quickly! I'll be able to test it later today :)

richvdh commented 5 months ago

For links, the result of the /search endpoint is specified at https://spec.matrix.org/v1.10/client-server-api/#_matrixclientv3search_results, and the count property is a member of Result Room Events.

count is indeed optional. It seems unfortunate (and probably not really intended?) that all of the properties in that structure, including results, are optional, but that's what the spec says.