Skip to content
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

fix: change spotify version needed in clientoken to use semantic format #1267

Merged
merged 1 commit into from
Mar 31, 2024

Conversation

acolombier
Copy link
Contributor

Fixes #1261

@acolombier acolombier force-pushed the fix/update-spotify-version branch from 6fe420b to a99a210 Compare March 19, 2024 22:05
@acolombier acolombier force-pushed the fix/update-spotify-version branch from a99a210 to d881f46 Compare March 19, 2024 22:05
@kingosticks
Copy link
Contributor

Which platforms are tested with this? Is it the string formatting that's important here or just the newer version number? It's a bit odd we still have the old version number in use elsewhere.

@acolombier
Copy link
Contributor Author

acolombier commented Mar 20, 2024

Good question - I'm not sure. I stumble across this as I was comparing the protobuf message between the Spotify Desktop Client and Librespot and noticed that the version appeared to be corrupted - it didn't include a string for a float32 instead, which I think explain the Bad Request server-side. Now, since spotify_version() returns a String, it is very odd how this ends up being packed as a float, instead of string - perhaps a bug in the proto crate?
Anyway, using a str as source seems to have fixed the problem, and I can now see identical proto messages between both desktop client and librespot. I didn't spend the time checking whether other values semantic version compatible would work, but I would expect so, if the theory of incorrect message format is the correct one.

My guess is that protocol version and client version are two separate things, but I'm not sure.

Edit: tested on Linux amd64

@wisp3rwind
Copy link
Contributor

FWIW, this also silences the warnings from #1261 for me (also Linux amd64).

@roderickvd
Copy link
Member

I'll merge it as it's clearly an important "hot fix". Thanks.

Ideally though all the number versions should be consistent with the protocol version at that time, meaning that the protobufs match / were extracted from that client version. If someone could do that...

@roderickvd roderickvd merged commit 9929635 into librespot-org:dev Mar 31, 2024
8 of 9 checks passed
@acolombier
Copy link
Contributor Author

I'd be happy to give it a go, but not sure how to do that. Is there wiki or doc? Or is that a matter of digging in Spotify files to find where they have been packed?

@hrkfdn
Copy link
Contributor

hrkfdn commented Apr 15, 2024

Is this something that's worth backporting to the master branch? It seems to be a problem with the current release.

@kingosticks
Copy link
Contributor

Are you sure? The current release is 0.4.2 and doesn't use a client token to get metadata via HTTP, it uses Mercury. It would be impossible to backport this fix to master. I'm not saying there isn't a similar problem in master but it seems unlikely to be exactly this.

@hrkfdn
Copy link
Contributor

hrkfdn commented Apr 15, 2024

Librespot doesn't as a binary. ncspot however uses it as a library and requests a client token via Mercury like so: https://github.com/hrkfdn/ncspot/blob/d1ea7b069efd4df7e311be9d6d24d4fa3edb2991/src/spotify_worker.rs#L66-L87

This token is then used to request HTTP metadata and according to the bug report hrkfdn/ncspot#1421 seems to be suffering from the same issue reported in #1261.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to get client token
5 participants