search

librespot-org / librespot

Open Source Spotify client library
MIT License
4.52k stars 544 forks source link

Parse expiry timestamp from spotifycdn.com CDN URLs (Fixes #1182) #1183

Closed kingosticks closed 1 year ago

kingosticks commented 1 year ago

The CDN URLs list now includes spotifycdn.com which has a different format. It was being erroneously interpreted using the scdn.co format and trying to parse non-digit characters as a timestamp.

It was trying to interpret the entire query string of "https://audio-gm-off.spotifycdn.com/audio/4712bc9e47f7feb4ee3450ef2bb545e1d83c3d54?Expires=1688165560~FullPath~hmac=IIZA28qptl8cuGLq15-SjHKHtLoxzpy_6r_JpAU4MfM=" as a number.

Maybe it would be better to match on the domain? Or at least catch the parse() error and just pretend it's not expiring?

roderickvd commented 1 year ago

Ah thanks for this! Is this ready to merge now or do you first need an answer to your question? I do agree we should make it more robust now it is clear that domains can change anytime. A catch-all seems like a good idea.

kingosticks commented 1 year ago

The other idea would be to use a regex and grab the first run of digits in the query string. That would catch all three versions and possibly future versions too.

I wasn't sure what the implication of ignoring the expiry was. Perhaps it's actually better to just ignore the URLs we can't parse. I'd be happy to make that extra change.

roderickvd commented 1 year ago

Maybe make a best effort attempt and on failure log a helpful message to report this unknown format to the project for implementation?

The expiry should only hit you when you somehow repeat a song, loading it from the same URL with the URL cached but not the file. Or you get the URL and then wait a really long time before downloading it. Should be edge cases, so the expiry is a nice to have.

kingosticks commented 1 year ago

I've had a go, what do you think?

roderickvd commented 1 year ago

Looks good to me, ready to merge?

kingosticks commented 1 year ago

Yep!