deniscerri / ytdlnis

Android Video/Audio Downloader app using yt-dlp
GNU General Public License v3.0
3.9k stars 138 forks source link

Question on permissions #393

Closed IzzySoft closed 4 months ago

IzzySoft commented 8 months ago

On today's update, my scanner reported:

! repo/com.deniscerri.ytdl_107020004.apk declares sensitive permission(s): android.permission.READ_EXTERNAL_STORAGE android.permission.READ_MEDIA_VIDEO android.permission.READ_MEDIA_AUDIO android.permission.MANAGE_EXTERNAL_STORAGE android.permission.REQUEST_INSTALL_PACKAGES

Most of those I think I could identify (please correct me where I'm wrong – and be welcome to fill in for the others):

image

But what is REQUEST_INSTALL_PACKAGES for? Which apps does it want to install?

As for DEPENDENCY_INFO_BLOCK, this is probably easy to get rid of:

android {
    dependenciesInfo {
        // Disables dependency metadata when building APKs.
        includeInApk = false
        // Disables dependency metadata when building Android App Bundles.
        includeInBundle = false
    }
}

For some background: that BLOB is supposed to be just a binary representation of your app's dependency tree. But as it's encrypted with a public key belonging to Google, only Google can read it – and nobody else can even verify what it really contains.

aicynide commented 8 months ago

@IzzySoft REQUEST_INSTALL_PACKAGES is for updating the app. Since, i couldn't update from your repo, i updated it from settings.

IzzySoft commented 8 months ago

Thanks @aicynide!

REQUEST_INSTALL_PACKAGES is for updating the app.

A self-updater? Is it opt-in – or enabled by default? Does it inform about where the APK is downloaded from?

Since, i couldn't update from your repo, i updated it from settings.

Did you receive an error when trying to update from my repo – or what happened? If there's a problem on my end, I should see to get that fixed ASAP.

aicynide commented 8 months ago

I mean last version in your repo is 1.7.2

IzzySoft commented 8 months ago

I mean last version in your repo is 1.7.2

Which came in here just 2 days ago, yes. And which happens to be the latest release available here as well. So what's wrong with it?

teddysulaimanGL commented 8 months ago

But what is REQUEST_INSTALL_PACKAGES for? Which apps does it want to install?

Hey, @IzzySoft, as far as I know, yes, this permission is newly added for some reason. And yes, I think it's for built-in app updater, but I won't be certain. The app has it for a while now, and it can be triggered by pressing the app version number from Settings

teddysulaimanGL commented 8 months ago

As per where it takes the APK from, I'm pretty sure it's directly from source code repo

zaednasr commented 8 months ago

Request install packages has been there for a while. Same with the self updater. The apk it uses are from the GitHub repo

IzzySoft commented 8 months ago

@teddysulaimanGL @zaednasr thanks! So it's not running automatically, but only when explicitly triggered by hand? That's "opt-in" then, good.

Background of the question are the inclusion criteria with my repo (which in this regard match those of F-Droid.org). Quote:

The app … 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 the checks performed in this repo if they activate it.

So I see opt-in is given. @zaednasr could you include a short explanation so it's clear where the APK is taken from? Then I'd add this to the app's "allow-list" here, including a short explanation with the permission (as shown in above screenshot). The DEPENDENCY_INFO_BLOCK currently is just shown for transparency; that hint/info would go away if the block is gone – which would be good but is not a "must" :wink:

teddysulaimanGL commented 8 months ago

So it's not running automatically, but only when explicitly triggered by hand? That's "opt-in" then, good.

Yes. But there's actually an option to automatic update check when opening the app which is enabled by default, and that's it. The app will still show you a confirmation dialog whether you want to proceed or not either way

zaednasr commented 8 months ago

The apk is taken straight from the github repo using the github api. You can go to utils. Update util to see for yourself

IzzySoft commented 8 months ago

I believe you, @zaednasr – but could that be made transparent, and the setting opt-in? It's reaching out to a non-free network service (yeah, so is grabbing YT content, but that one is clear to those using the app as they installed it because of that) which not everyone is comfortable with :wink:

As for the setting: it could e.g. be initially set to NULL – and when NULL, the app could ask for whether to turn it on or off. If at that point the context (checked at/downloaded from Github directly etc) is made clear, that's what GDPR calls "explicit and informed consent" :stuck_out_tongue_winking_eye: Thanks in advance!

zaednasr commented 8 months ago

The option to turn the auto updater off is present in the updating setting category.

The reason is on by default is so users are not stuck in older versions as there are constant changes and bug fixes.

I could note it down that the auto updater gets the apk from github no problem

IzzySoft commented 8 months ago

The option to turn the auto updater off is present in the updating setting category.

Yes, but the policy in F-Droid repos (at least in mine and at fdroidorg) requires it to be opt-in, not opt-out. Maybe you could at least check the installer source (see get the installer source of an android app), and only auto-enable it if that is either the app itself (after a self-update) or Android's package installer (when installed directly from a Github download)? If it's something else (F-Droid client, Obtainium, …) the user obviously wants to use that for update management.

The reason is on by default is so users are not stuck in older versions as there are constant changes and bug fixes.

Understood – and that would be covered this way as well.

I could note it down that the auto updater gets the apk from github no problem

That would be a good start, thanks!

zaednasr commented 8 months ago

I didn't get the installer option part. The apps auto updater doesn't auto install. Just pops a dialog that a new update is available. He can also ignore that dialog preventing it from showing up for that version until a new one comes up

After he agrees, the app will download the apk and even then the user still has to install it himself. The apk is stored in downloads.

There is no action done without the user not knowing it

IzzySoft commented 8 months ago

@zaednasr what you describe does not need REQUEST_INSTALL_PACKAGES then as installation would happen via "download notification" – or do I miss something? Apart from that, did the user consent to the update check itself?

Consent means it needs to be opt-in […] and structured in a way that clearly explains to users that they’re choosing to bypass the checks performed in this repo if they activate it.

No offense meant; as someone not using the app myself I might simply miss something here. And I won't "argue" if that information is at least clear at the time of the download itself (gray zone maybe – but I'm neither a moralist nor a bureaucrat :wink:). Just pointing out how a "clean solution" would IMHO look like.

And to be clear on that point as well: I'm not, repeat, not saying you would want to "sneak something in" there. I tend to trust the authors of apps I include with my repo. But mistakes happen to the best of us – and it wouldn't be the first time that my scanners caught something brought in by e.g. some dependency. So it's an additional screening for the benefit of all – which everyone is free to bypass, but should be aware of the implications :wink:

zaednasr commented 8 months ago

I understand. I believe that permission was added in early development during experimentation. Will remove

zaednasr commented 8 months ago

Added the note and removed the permission in the latest release

IzzySoft commented 7 months ago

Thanks! No warning about the permission anymore. The only thing left now is:

! repo/com.deniscerri.ytdl_107030004.apk contains signature block blobs: 0x504b4453 (DEPENDENCY_INFO_BLOCK; GOOGLE)

Looks like you forgot to include those lines with your build.gradle?

zaednasr commented 7 months ago

oh right, my bad. Yes ill add them now

Incog5 commented 4 months ago

Awesome app! Would it be possible to remove the excess permissions to all users photos and videos, which is a privacy concern for apps that have internet access. Similar functioning apps work without these permissions. Screenshot_20240607_083602 Screenshot_20240607_080108_Permission controller Screenshot_20240607_083020_Permission controller Screenshot_20240607_080127_Permission controller

zaednasr commented 4 months ago

Will look into this

IzzySoft commented 4 months ago

@zaednasr as your app requires at least Android 6, you could look into using Storage Access Framework (SAF) for this and let the user pick a location for download. Some locations would be off-limit then, though, including…

Same intro: Since Android 6, storage access has become a runtime permission. So @Incog5, even if an app declares it in its manifest, it doesn't automatically get them. Runtime permissions (aka AppOps) require the "user¹" to explicitly confirm them – meaning the app must fire up a dialog asking, which needs to be confirmed or the app would not get access to storage.

But you're correct of course, this would be a dilemma: You would need to grant those permissions in order to save downloaded files – but the permission would include almost the entire storage. While with SAF, you pick the location and the app only gets access to the location you selected.

¹ I try to avoid terms like "user" and "consumer" as it degrades humans to object, but sometimes it's hard to if it should be easy to understand, so apologies.

Incog5 commented 4 months ago

To follow up, Seal is able to download to its default download directories without granting any access, no popup or SAF selection.

Video folder /storage/emulated/0/Download/Seal Audio folder /storage/emulated/0/Download/Seal/Audio

Newpipe requires user to select the folder, while Downloads can't be used, subdirectories inside Downloads can.

@IzzySoft you can test this, Seal will create it's own subdirectory in Downloads on app launch, are subdirectories that an app creates in Downloads exempt from user opt-in permissions?

IzzySoft commented 4 months ago

/storage/emulated/0/Download/Seal

Ah, right! Yes, I've heard that. That should work, just directly into Downloads/ would not.

while Downloads can't be used, subdirectories inside Downloads can.

That ^^

you can test this, Seal will create it's own subdirectory

I can't, no: neither am I using Seal, nor am I a developer. So this question should rather go to @zaednasr or another contributor of the project :wink: I'm just the founder and maintainer of the IzzyOnDroid repo, where this app is listed at.

zaednasr commented 4 months ago

I believe this is more likely an issue for API 30 and above. because i tested for api 23-29 and they all needed the write/read permissions to actually move the finished file from the data internal directory to the downloads/ytdlnis folder. The app couldnt even tell if the file existed or not. So it needs to be there

For 30 and above i removed the permissions as the app appears to be working without them

IzzySoft commented 4 months ago

For 30 and above i removed the permissions as the app appears to be working without them

Thanks! I still need to update the scanner to check for maxSdk on permissions…

Btw, today the scanner found another permission added: android.permission.SYSTEM_ALERT_WINDOW. Can you please give me the background, so I can add that to the "green list" as well?

zaednasr commented 4 months ago

I am experimenting with it. You know how the app shows the popup card when you share a link from youtube. Well in revanced, youtube goes picture in picture instead of just staying in the background. Newpipe has that permission but it amounted to nothing. Ill have to investigate further how to achieve it.

IzzySoft commented 4 months ago

OK, thanks – then I'll leave the warning color in for now until you decided. Just let me know then what to put up – if it's still needed that is.

zaednasr commented 4 months ago

I ended up removing the permission for now in the latest release 1.7.7

IzzySoft commented 4 months ago

Great! Then the warning will disappear automatically once the new release has been pulled.