CappielloAntonio / tempo

An open source and lightweight music client for Subsonic, designed and built natively for Android.
GNU General Public License v3.0
1.18k stars 53 forks source link

Calls to getCoverArt (and possibly other API methods) don't URL-encode the username #41

Closed gravelld closed 1 year ago

gravelld commented 1 year ago

I received a support request for someone testing Tempo against Astiga. They were able to connect to Astiga and browse their library, but unable to show cover art or stream music. This was odd because when I tested Tempo it worked for me.

Trying to replicate, I found I was able to do so when I replicated their email address, which has a "+" suffix many people use to file their email, e.g. myname+astiga@fastmail.com . I tested this with Postman and sure enough the call didn't work:

{{host}}/rest/getCoverArt?u=myname+astiga@fastmail.com&s=...&t=...&v=1.15.0&c=Tempo&id=109508

When I manually URL encoded the username it worked fine:

{{host}}/rest/getCoverArt?u=myname%2bastiga@fastmail.com&s=...&t=...&v=1.15.0&c=Tempo&id=109508

In the Astiga code, if I output the username without the encoding I get myname astiga rather than myname+astiga - that's because "+" is typically used to mean a space in a query string.

Looking at the Tempo code, URLs and calls for getCoverArt appear to be built here: https://github.com/CappielloAntonio/tempo/blob/91d91d30241866aa0d0647dbc13a36084ebb62e2/app/src/main/java/com/cappielloantonio/tempo/glide/CustomGlideRequest.java#L49 . There's no URL encoding I can see.

I think this should be encoded to %2b really - what do you think?

If you don't want to do that, is there a workaround by somehow configuring a different authentication approach?

As I mentioned this also appears to affect playback. Browsing the library works fine.

CappielloAntonio commented 1 year ago

Hi @gravelld, thanks for reporting.

Apparently Retrofit, the library to manage the network, applied a manual encode before making each request, hence the correct operation of the navigation. On the contrary, Glide (image loader) and Media3 (trivially the media player), use the original uri. Good to know!

I'm about to post a pre-release build. From my local tests everything still works, but it's always better to test in more users.

CappielloAntonio commented 1 year ago

New version released, waiting for your test results. If the problem is solved you can close this issue.

gravelld commented 1 year ago

I tried the new build, but when I add a new server and press it to view the library the app seems to crash. Restarting Tempo seems to crash too.

CappielloAntonio commented 1 year ago

Well, that's very strange. Can you pass me the crash log or give me a chance to try it myself?

gravelld commented 1 year ago

I'm not sure how else to get you that other than logcat, let me know if there's a better way.

Here's what I found that reference the Tempo pid.

fwiw the user that originally reported this issue experiences a crash on startup too.

CappielloAntonio commented 1 year ago

Does Astiga have a demo instance to replicate the problem with?

gravelld commented 1 year ago

It does; can you get in touch via email?

I don't think it's even getting that far though...

CappielloAntonio commented 1 year ago

This is my junk mail: email

Not being able to reproduce the crash with navidrome, gonic and now airsonic-advanced, I'd like to try with Astiga.

gravelld commented 1 year ago

Oh, I assumed the app was just crashing on startup and not even making a HTTP call. I've sent the email.

CappielloAntonio commented 1 year ago

Hi @gravelld, even with the demo user on the Astiga instance I can't reproduce the error. Unfortunately, the logcat you sent me is unreadable as the app is signed with the release keys.

Could you try this apk and take another logcat? This is the version published on F-Droid (but not signed), it is the same but without Google services. This way I don't overwrite the already installed version of the app.

app-notquitemy-debug.zip

In the last step, I would like you to clear the data of the current app and start from scratch.

gravelld commented 1 year ago

Well I'm sure this will surprise you (or maybe not) but the above APK works fine for me. Also, the original bug is fixed.

Not sure what was up with the APK previous. I was installing it by downloading it from Github with my device and then installing it on the same device by launching the APK.

CappielloAntonio commented 1 year ago

Well, yes... The code of the two versions hasn't changed much. Could you try, if it's not a problem, to download the latest version (v3.5.2) which I will upload shortly, delete the old data and try to access again?

gravelld commented 1 year ago

I'm afraid the same thing happens - it crashes as soon as I press the Subsonic server I configured. I tried installing both on the phone and via adb.

CappielloAntonio commented 1 year ago

Okay, I guess I just don't know what to think anymore. The last alternative is to proceed with the unsigned app installation and grab another logcat at the time of the crash.

app-tempo-debug.zip

gravelld commented 1 year ago

That one starts fine... I've attached the log anyway.

tempo-startup.log

Looks like you can see some decipherable stack traces which might map to the original ones in the version that crashed.

Could it be an Android version issue?

CappielloAntonio commented 1 year ago

Okay, maybe you gave me an idea

CappielloAntonio commented 1 year ago

I'm so sure (almost) and hopeful that I've fixed the problem, that I'm releasing a new release without waiting for the test results (please test it and give me good news).

gravelld commented 1 year ago

Yes, that works! And the original bug is solved too! I'll mark this closed, but as a novice Android developer myself (trying to maintain the Astiga Android app) I'd like to know what the issue was for my own learning...

CappielloAntonio commented 1 year ago

Actually the fact that the signed app didn't work, while the unsigned app worked was a big hint.

I admit that I don't often try both versions because it is in continuous development, so the app I use is the debug one built by Android Studio.

Another issue that threw me off track is the fact that for you the app first worked and then didn't (without having changed anything about the networking part). And this is still a mystery to be figured out.

In short, I solved it by excluding Retrofit (the library that manages the network part) from the obfuscation of ProGuard. At this moment the exclusion is total, it could be further refined.

-keep class retrofit2.** { *; }

What I'm saying with this rule is "keep everything as it is, don't obfuscate anything" pretty much.