New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Protect against clients using the search API incorrectly. #1067

Closed
adamcik opened this Issue Mar 22, 2015 · 4 comments

Comments

3 participants
@adamcik
Member

adamcik commented Mar 22, 2015

Playing around with some things today I found out that mopidy-mobile seems to send searches incorrectly. Mopidy-Spotify see it as a search for "f" "o" "o" " " "f" "i" "g" "h" "t" "e" "r" "s" while, Mopidy-Local-Sqlite seems to handle it "correctly". I suspect we want to normalize the query in core so that we at least call the backends in a consistent way.

So input of "query":{"any":"foo fighters"} should log a warning and convert the query to "query":{"any":["foo fighters"]}

Additionally, we should check if there are bugs related to this in mopidy-mobile and/or mopidy-local-sqlite as I think they differ from the intended behavior for the API.

/me looks forward to switching to a new search API sometime in the future.

2015-03-21 23:32:53,642 DEBUG    [HttpServer] /home/adamcik/dev/mopidy/mopidy/http/handlers.py:115
  mopidy.http.handlers Received WebSocket message from 127.0.0.1: u'{"method":"core.library.search","params":{"query":{"any":"foo fighters"},"uris":null},"jsonrpc":"2.0","id":1}'
2015-03-21 23:32:53,644 DEBUG    [LocalBackend-3] /home/adamcik/dev/mopidy-virtualenv/local/lib/python2.7/site-packages/mopidy_local_sqlite/schema.py:224
  mopidy_local_sqlite.schema SQLite search query [u'foo fighters']: 
SELECT *
  FROM tracks
 WHERE docid IN (SELECT docid FROM fts WHERE fts MATCH ?)

2015-03-21 23:32:53,747 DEBUG    [Core-9] /home/adamcik/dev/mopidy-spotify-2/mopidy_spotify/utils.py:15
  mopidy_spotify.utils Playlist fetch took 101ms
2015-03-21 23:32:53,747 DEBUG    [SpotifyBackend-5] /home/adamcik/dev/mopidy-spotify-2/mopidy_spotify/library.py:230
  mopidy_spotify.library Searching Spotify for: "f" "o" "o" " " "f" "i" "g" "h" "t" "e" "r" "s"
@tkem

This comment has been minimized.

Member

tkem commented Mar 22, 2015

Question is, whether queries should be normalized (as currently done by Mopidy-Local-SQLite et al.) or if these should fail with a meaningful error message so clients are forced to fix this.

@tkem

This comment has been minimized.

Member

tkem commented Mar 22, 2015

BTW, I think I got this habit of "normalizing" queries in the backend from
https://github.com/mopidy/mopidy/blob/develop/mopidy/local/search.py#L137
so maybe that should be changed, too.

@jodal jodal added the 1 - Ready label Mar 22, 2015

@adamcik

This comment has been minimized.

Member

adamcik commented Mar 22, 2015

Indeed, that code should also be removed.

@adamcik adamcik self-assigned this Mar 22, 2015

@jodal jodal added 2 - Working and removed 1 - Ready labels Mar 22, 2015

@adamcik

This comment has been minimized.

Member

adamcik commented Mar 22, 2015

Fixed by #1073

@adamcik adamcik closed this Mar 22, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment