fast4x / RiMusic

A multilingual Android application for streaming music from YouTube Music.
https://rimusic.xyz
GNU General Public License v3.0
2.13k stars 110 forks source link

Questions on permissions requested by the RiMusic app #400

Closed IzzySoft closed 6 months ago

IzzySoft commented 8 months ago

Steps to reproduce the bug

Look at the permissions (either in Android's app list, or at the AndroidManifest.xml

Expected behavior

According to the app description, RiMusic is supposed to play audio – but not to record it, so requesting this permission is quite "unexpected behavior" :wink: and it should rather not be requested.

Actual behavior

The permission is declared in the Manifest.

Screenshots/Screen recordings

n/a

Logs

n/a

RiMusic version

0.6.17

Android version

any

Additional information

Not complaining, but this looks weird and might raise suspicions. So could you please remove this permission – or if it's needed indeed, clarify that in the app description (Fastlane, Readme etc)?

IzzySoft commented 8 months ago

PS: I see this issue got the number 400, so HTTP respond code seems to concur this is a "400 Bad Request" :see_no_evil:

fast4x commented 8 months ago

Hi Izzy, permission is only required to use the visualizer. By default the visualizer is disabled in the settings. When the user activates it because he wants to use it, permission is required and the user is free not to grant it.

I immediately add the details in the readme, fastlane and wiki FAQ

ikanakova commented 8 months ago

@fast4x I think it would be great to list all the permissions the app requires and why.

fast4x commented 8 months ago

@fast4x I think it would be great to list all the permissions the app requires and why.

Good idea

ikanakova commented 8 months ago

I think that's enough to be listed in the fastlane and FAQs. And mention in the readme with a link to the wiki.

fast4x commented 8 months ago

Yes

fast4x commented 8 months ago

All updated, readme and fastlane. Here faqs page with all permissione https://github.com/fast4x/RiMusic/wiki/FAQs#-what-permissions-are-used

IzzySoft commented 8 months ago

First thanks for the quick action! I still wonder:

permission is only required to use the visualizer

Why does a visualizer require access to the microphone? It's not called "audioliser". Visuals are to watch, not to "talk to" :flushed: Can you help me understand what I miss there? I guess I'm not the only one "with a knot in the head" there, so maybe a hint in the Wiki (yay, great idea to list all permissions there, @ikanakova!) would be good. Oh, and maybe a hint in the Fastlane full_description.txt that permissions are explained in the (link) Wiki?

fast4x commented 8 months ago

I added the link to the wiki in the fastlane, I thought I already did.

According to the official visualizer documentation: "The Visualizer class enables application to retrieve part of the currently playing audio for visualization purpose. It is not an audio recording interface and only returns partial and low quality audio content. However, to protect privacy of certain audio data (e.g voice mail) the use of the visualizer requires the permission android.permission.RECORD_AUDIO."

https://developer.android.com/reference/android/media/audiofx/Visualizer

IzzySoft commented 8 months ago

Hm, K. In a way, it is "recording", yeah. Confusing though, as it belongs to the permission group "Microphone" ("Direct access to the microphone to record audio."), see here. But it seems it was removed from that group at some point, and now is "undefined". Well, "the ways of Google are inscrutable"…

Thanks a lot! I've added it to the allow-list for your app now, pointing to the developer link you just quoted. Thanks also for updating the full description in fastlane once more! All done then here, so I "close the door" now. Should there be any questions left for me on your end, just give me a ping :smile:

fast4x commented 8 months ago

Ping for you 😂 Thanks for your attention, I like this kind of interaction. I have a question about the F-Droid build process for you if you can give me an indication.

It’s happening that I publish the new version on GitHub, load the apk built with the same committed source, but F-Droid does not build from this tag because it finds differences. This thing happens from a couple of versions. The tag and apk are correct, I don’t understand.

F-Droid creates from the tag, I do not understand why it finds differences.

ikanakova commented 8 months ago

https://developer.android.com/reference/android/media/audiofx/Visualizer

I've added this link to the wiki as well to make it clear that this is an Android invention.

IzzySoft commented 8 months ago

@fast4x which differences? See HOWTO: diff & fix APKs for Reproducible Builds. There can be any of several reasons. Example: You build on Windows, which generates files with \r\n as EOL. F-Droid builds on Linux, which uses \n. Same source, different results. Other examples behind the link – including how to solve them (if possible).

fast4x commented 8 months ago

Perfect! I’ll look at it right away, thank you!

fast4x commented 8 months ago

397 it's the last reported difference I’m trying to figure out.

IzzySoft commented 8 months ago

Help is on its way, see my comment there (I was performing those RB tests often while I was active at F-Droid, hence I have the proper tools at hand – that diff.html is just "too much input" to be helpful for someone not accustomed with the task).

IzzySoft commented 8 months ago

OK, next batch (with all checks active now in my scanner):

! repo/it.fast4x.rimusic_14.apk declares risky permission(s): android.permission.READ_EXTERNAL_STORAGE android.permission.READ_MEDIA_AUDIO

I guess we can add READ_MEDIA_AUDIO to the allow-list straight away, as your music app has a good reason to want that (play audio stored there). But why READ_EXTERNAL_STORAGE? Minimum Android version needed to use your app is Android 5, which already has SAF (Storage Access Framework) available, so there should be no need for the older access methods. Or do I miss something here?

Thanks ahead once more for shedding light!

fast4x commented 8 months ago

Hi Izzy, you’re right, actually that permission (READ_EXTERNAL...) was already there since i forked ViMusic. I remove It soon.

The app also requires WRITE_EXTERNAL_STORAGE permission which is used up to Android 10 to set custom cache.

READ_MEDIA_AUDIO it's used for read local songs...

I always thank you for your kindness.

fast4x commented 8 months ago

Confirmed, i have removed READ_EXTERNAL_STORAGE and WRITE_EXTERNAL_STORAGE not needed.

fast4x commented 8 months ago

Also updated wiki page https://github.com/fast4x/RiMusic/wiki/FAQs#-what-permissions-are-used

IzzySoft commented 8 months ago

Thanks a lot! Added READ_MEDIA_AUDIO to your app's allow-list. The other two should then be gone with the next release and thus need now allow-listing.

IzzySoft commented 8 months ago

@fast4x looks like that permission list needs an update. Today my scanner reported additional things:

ikanakova commented 8 months ago

@fast4x looks like that permission list needs an update. Today my scanner reported additional things:

android.permission.REQUEST_INSTALL_PACKAGES: what for does a music player need to install apps? usesCleartextTraffic: where are unencrypted connections established to?

I think this is related to issue #555

fast4x commented 8 months ago

Hi @IzzySoft, from this version check update feature include download and install package from GitHub, when user click Update button. #555

Library in use is AppUpdate, look at this GitHub link https://github.com/azhon/AppUpdate

All connections to GitHub or YT/YTM from app are in https, not have connection unencrypted.

fast4x commented 8 months ago

Updated permissions list in the wiki page https://github.com/fast4x/RiMusic/wiki/FAQs#-what-permissions-are-used

IzzySoft commented 8 months ago

check update feature include download and install package from GitHub

Thanks @fast4x! So the app now has a self-updater? You're aware that violates the inclusion policy of F-Droid and my repo as well (as in both cases, the additional screening applied by the repos is skipped)?

when user click Update button

So it's kinda opt-in then (update check only happens when that button is pressed)? Is it clear to someone not having followed discussions here that updates then happen directly from Github – and are the implications (circumventing the extra security provided by F-Droid/my repo) clearly pointed out?

All connections to GitHub or YT/YTM from app are in https, not have connection unencrypted.

Then why does the app have the usesCleartextTraffic set?

<application android:theme="@7F0F011C" android:label="RiMusic" android:icon="@7F0D0001"
 android:name="it.vfsfitvnm.vimusic.MainApplication" android:configChanges="0x40007FFF"
 android:allowBackup="true" android:hardwareAccelerated="true" android:supportsRtl="true"
 android:banner="@7F0D0000" android:extractNativeLibs="true" android:usesCleartextTraffic="true"

Maybe some dependency merged that in, and you could "override" it?

DarkCrypt commented 8 months ago

@fast4x My apologies friend, I didn't mean for you to do pointless work.

@IzzySoft I'm the one who requested the in-app updater feature. I didn't realize this was going to cause such conflict. This is extremely upsetting.

fast4x commented 8 months ago

check update feature include download and install package from GitHub

Thanks @fast4x! So the app now has a self-updater? You're aware that violates the inclusion policy of F-Droid and my repo as well (as in both cases, the additional screening applied by the repos is skipped)?

when user click Update button

So it's kinda opt-in then (update check only happens when that button is pressed)? Is it clear to someone not having followed discussions here that updates then happen directly from Github – and are the implications (circumventing the extra security provided by F-Droid/my repo) clearly pointed out?

All connections to GitHub or YT/YTM from app are in https, not have connection unencrypted.

Then why does the app have the usesCleartextTraffic set?

<application android:theme="@7F0F011C" android:label="RiMusic" android:icon="@7F0D0001"
 android:name="it.vfsfitvnm.vimusic.MainApplication" android:configChanges="0x40007FFF"
 android:allowBackup="true" android:hardwareAccelerated="true" android:supportsRtl="true"
 android:banner="@7F0D0000" android:extractNativeLibs="true" android:usesCleartextTraffic="true"

Maybe some dependency merged that in, and you could "override" it?

@IzzySoft Related to cleartext i had it enabled for debugging, I forgot to disable it. I'm afraid, disable it and rebuild.

Check update, check if there is an update on startup, if there is shows the user a message with an update button. Only if the user clicks Update start download and installation which still requires the user to confirm further.

This violates FDroid policy? Aigoo...

fast4x commented 8 months ago

@IzzySoft https://gitlab.com/fdroid/fdroiddata/-/issues/3110#note_1613431934

Could I make it configurable from the settings if I use the simple check update or the new one that also just with explicit user consent, allows you to download and launch the installation.

So by default I would have the verification as before, the user could activate the new mode from the settings, in both cases the download is still from github as before.

fast4x commented 8 months ago

@IzzySoft The software must not download additional executable binary files (e.g. addons, auto-updates, etc.) without explicit user consent. Consent means it needs to be opt-in (it must not be harder to decline than to accept or presented in a way users are likely to press accept without reading) and structured in a way that clearly explains to users that they’re choosing to bypass F-Droid’s checks if they activate it.

In the settings I could explain well what the policy requires when user enabled new check update...

what do you think?

IzzySoft commented 8 months ago

@DarkCrypt the updater is useful for those installing directly from Github. But as outlined, it has some implications for those trusting on the extra screening provided e.g. by my repo. They install the app via their F-Droid client, and expect updates come from the same source they installed from. So if they e.g. receive an update prompt, or push some update button, that's where they expect the update to come from. Not from Github or any other place, but from where they installed the app from.

Solutions to that include a) a separate build (so my repo can pick the build without the updater), b) the updater to be opt-in with clear information on these details before it gets enabled. a) is preferable, b) is acceptable.

@fast4x

Check update, check if there is an update on startup, if there is shows the user a message with an update button. Only if the user clicks Update start download and installation which still requires the user to confirm further.

That is border-line. The entire check should only happen with "explicit and informed consent". What you describe could be seen as consent for the update, but not for the check (reaching out to a non-free network without consent is kind of a red flag for some privacy folks – and rightfully so). And the "informed" part is amiss here – so yes, it violates the policy.

Could I make it configurable from the settings if I use the simple check update or the new one that also just with explicit user consent, allows you to download and launch the installation.

That sounds good. Suggestion: have a new setting item for that, initialized with NULL. When starting the app and that field is set to NULL, inform about the updater mentioning source and implication and ask whether to have in enabled or disabled. Then remember that setting (and act accordingly). The option in settings of course remains accessible should one have a change of mind.

In the settings I could explain well what the policy requires when user enabled new check update...

Yupp: that updates are checked for as well as downloaded from Github releases, and thus wouldn't receive additional screening/security checks by e.g. F-Droid or IzzyOnDroid. That compared to IzzyOnDroid the update would at best happen a few hours earlier (as my updater fetches them daily) might be a good hint, too – for F-Droid it would be days earlier.

Related to cleartext i had it enabled for debugging, I forgot to disable it. I'm afraid, disable it and rebuild.

So it will be gone with the next release, solves it for me. No urgent need to replace the existing version only for that; whoever has it already installed wouldn't receive that update anyway.

Thanks to both of you!

fast4x commented 8 months ago

check update feature include download and install package from GitHub

Thanks @fast4x! So the app now has a self-updater? You're aware that violates the inclusion policy of F-Droid and my repo as well (as in both cases, the additional screening applied by the repos is skipped)?

when user click Update button

So it's kinda opt-in then (update check only happens when that button is pressed)? Is it clear to someone not having followed discussions here that updates then happen directly from Github – and are the implications (circumventing the extra security provided by F-Droid/my repo) clearly pointed out?

All connections to GitHub or YT/YTM from app are in https, not have connection unencrypted.

Then why does the app have the usesCleartextTraffic set?

<application android:theme="@7F0F011C" android:label="RiMusic" android:icon="@7F0D0001"
 android:name="it.vfsfitvnm.vimusic.MainApplication" android:configChanges="0x40007FFF"
 android:allowBackup="true" android:hardwareAccelerated="true" android:supportsRtl="true"
 android:banner="@7F0D0000" android:extractNativeLibs="true" android:usesCleartextTraffic="true"

Maybe some dependency merged that in, and you could "override" it?

@IzzySoft Related to cleartext i had it enabled for debugging, I forgot to disable it. I'm afraid, disable it and rebuild.

Check update, check if there is an update on startup, if there is shows the user a message with an update button. Only if the user clicks Update start download and installation which still requires the user to confirm further.

This violates FDroid policy? Aigoo...

I checked my manifest, I had already removed usesCleartextTraffic, I found where it is reactivated. It’s the AppUpdate package that requires it in its manifest https://github.com/azhon/AppUpdate/blob/main/appupdate/src/main/AndroidManifest.xml Then I remove the package and go back to the previous check update method.

Thanks @IzzySoft for your control and support

fast4x commented 8 months ago

@DarkCrypt i'm afraid, i will develop new version without other permission request

fast4x commented 8 months ago

@fast4x looks like that permission list needs an update. Today my scanner reported additional things: android.permission.REQUEST_INSTALL_PACKAGES: what for does a music player need to install apps? usesCleartextTraffic: where are unencrypted connections established to?

I think this is related to issue #555

I removed package, i will develop new function without permission in the future

ikanakova commented 8 months ago

@fast4x looks like that permission list needs an update. Today my scanner reported additional things: android.permission.REQUEST_INSTALL_PACKAGES: what for does a music player need to install apps? usesCleartextTraffic: where are unencrypted connections established to?

I think this is related to issue #555

I removed package, i will develop new function without permission in the future

I think it could be done by downloading a new version, but the user would have to manually install it themselves using the Files app, for example. Then this permission would not be needed.

fast4x commented 8 months ago

Using the system download service, the user should be able to manually launch the installation from the notification area. What do you think?

fast4x commented 8 months ago

Screenshot_20240204_001144 New check update with actions: open releases page and download with default policy without permission to install

fast4x commented 8 months ago

Screenshot_20240204_002453

With info for F-Droid users

IzzySoft commented 8 months ago

Yes, that looks fine! If you wish you could expand the last statement:

F-Droid users can wait for the update from F-Droid, which usually arrives within a few hours with the IzzyOnDroid repo, and a few days with F-Droid.org's repo.

Icing on the cake, agreed :rofl:

DarkCrypt commented 8 months ago

@IzzySoft I understand what's all being said here, thank you for clarifying. I assume all apps that have this feature within your repo and F-Droid's repo have a similar "warning"? I'm probably wrong but I don't recall ever seeing this.

@fast4x Again, I do apologize for the headache. I'm glad to know this feature can still exist because of these changes. Your implementation looks great! I mainly made this request in hopes it'll work for Android TV. Thank you for everything you do!

fast4x commented 8 months ago

Yes, that looks fine! If you wish you could expand the last statement:

F-Droid users can wait for the update from F-Droid, which usually arrives within a few hours with the IzzyOnDroid repo, and a few days with F-Droid.org's repo.

Icing on the cake, agreed :rofl:

Yes! Will be add icing on the cake 😂

fast4x commented 8 months ago

Screenshot_20240204_120701

I like it, very transparent and respectful to all types of users

IzzySoft commented 8 months ago

@DarkCrypt

I assume all apps that have this feature within your repo and F-Droid's repo have a similar "warning"? I'm probably wrong but I don't recall ever seeing this.

Yes. And yes; those additional checks were just implemented in January, which is why the results pop up only now. I reach out whenever an update triggers them. For existing apps, wherever you see those in the package details on the repo browser, be welcome to reach out to the corresponding project yourself to ask for clarification and ping me from the issue when clarifications have been made. I might make a "full scan" once reports from my updater have "calmed down" – but I cannot yet say when this might be.

IzzySoft commented 8 months ago

@fast4x Wonderful! :star_struck: So when that "popup" shows and I want "option 3" (wait for the F-Droid client to deal with the update), I tap that option (or the "back" key) to close it?

fast4x commented 8 months ago

@fast4x Wonderful! :star_struck: So when that "popup" shows and I want "option 3" (wait for the F-Droid client to deal with the update), I tap that option (or the "back" key) to close it?

The popup closes by touching any point outside it. 🥳

IzzySoft commented 8 months ago

Thanks a lot! Couldn't resist this one then: https://floss.social/@IzzyOnDroid/111873690047110470 Thanks! So with the next release, REQUEST_INSTALL_PACKAGES will be gon again, right? Then this issue can be closed, as everything has been addressed.

PS: The check itself is opt-in, too? Or does the app still check without consent? Because then it would need NonFreeNet, as it reaches out to a non-free network service.

fast4x commented 8 months ago

Thanks a lot! Couldn't resist this one then: https://floss.social/@IzzyOnDroid/111873690047110470 Thanks! So with the next release, REQUEST_INSTALL_PACKAGES will be gon again, right? Then this issue can be closed, as everything has been addressed.

PS: The check itself is opt-in, too? Or does the app still check without consent? Because then it would need NonFreeNet, as it reaches out to a non-free network service.

Beautiful! Thank you very much for using RiMusic as an example for everyone. It’s a nice prize for me.

REQUEST_INSTALL_PACKAGES no longer exists, so in the next version there will be no more.

fast4x commented 8 months ago

Checking for the presence of the new version on GitHub is not opt-in, it was not before. Do I have to make it opt-in under the new rules?

P.s. The popup is shown only if there is a new version on GitHub... obviously after checking if a new version has been published.

fast4x commented 8 months ago

Maybe it is more correct to turn on/off the control from the settings, tonight I add it Will be disabled by default. User can enable it from settings.

Update also wiki page with this news.

IzzySoft commented 8 months ago

Maybe it is more correct to turn on/off the control from the settings, tonight I add it Will be disabled by default. User can enable it from settings.

That sounds good! See my comment from yesterday, quote:

Suggestion: have a new setting item for that, initialized with NULL. When starting the app and that field is set to NULL, inform about the updater mentioning source and implication and ask whether to have in enabled or disabled. Then remember that setting (and act accordingly). The option in settings of course remains accessible should one have a change of mind.

Thanks to the latest updates, that can be shortened to something like "Check at Github for updates?" – though it cannot hurt to add "When an update is available, you will be asked if you want to install it. There won't be automated installs „behind your back“."