Why is usesCleartextTraffic set?

IzzySoft commented 7 months ago

Just wondering why you declared usesCleartextTraffic for your app (i.e. that it should be able to use non-secured http://). I could not find any reference here in your repo, but the AndroidManifest.xml in the APK-file declares it. Is there a need to access http:// only places?

Btw: Searching for http: in your repo showed some links which meanwhile should be https: – like the link to the Amazon AppStore and to TheTVDB in the Readme, links to Kodi in the FAQs and some more.

phhusson commented 7 months ago

There are many reasons. To name just a few: UPnP, webdav (although people should use webdav over https), or simply playing files requested by the user (if a user clicks from a non-browser on a http://xxxxxx/yy.mkv URL, they can open it in NOVA)

For instance, when you use NOVA with an external player it exposes a http server for that external player to read that file. To do the reverse, well we do need to be able to use http on localhost.

courville commented 7 months ago

Indeed, and also when playing network shares, they are exposed by nova to internal native avos player as http stream to not burden with non necessary cyphering (cf. FileCoreLibrary/src/com/archos/filecorelibrary/StreamOverHttp.java). @IzzySoft, thanks for the heads up on non https links to sites: I will fix this.

IzzySoft commented 7 months ago

Thanks! I assumed as much (from the references to Kodi), but wanted to make sure. Will add it to the "allow list" for Nova then. (For some background: I've just added some extra security checks with my repo, see here, so the flag popped up when your fresh release was being pulled :wink:)

IzzySoft commented 7 months ago

Oh, the issue is still open, so let me append a second finding (yeah, still expanding the checks step by step, next stage just fired). Today my scanner reported Nova requests android.permission.RECORD_AUDIO. So my first thought was: well, it's a video player – not a recorder. I have a vague idea again, but as the first time prefer to hear from you. I'm confident you once more will approve my guess and I can add that to the allow-list as well, but I need to make sure :wink:

phhusson commented 7 months ago

I seem to remember that we require it to be able to do voice search. I know it doesn't make much sense (since we are not the ones doing the voice to text, but Google app does it for us), but well. That being said, IIRC that's also amongst the permission we ask for way too eagerly, and we should stop to.

IzzySoft commented 7 months ago

Hm, so I guessed wrong then. Reading the part again that I had in mind: indeed, misread "(TV-friendly) Audio Boost mode" as "adjust volume if surrounding noise is interfering" :see_no_evil: So I concur it doesn't make much sense. Could you check if it can be dropped then (and everything still works the same)? Even if not always justified: RECORD_AUDIO together with INTERNET always has a "nasty connotation" (is someone "listening in"?) (definitely not saying you do :no_mouth:). So not being there in the first place is the best defense one has against that :wink:

IzzySoft commented 7 months ago

Apologies for bringing each thing up separately, but I'm extending those checks slowly to not get swamped by too many reports. Here's today's alert:

! repo/org.courville.nova_2585577.apk declares risky permissions: android.permission.SYSTEM_ALERT_WINDOW android.permission.RECORD_AUDIO

We already addresses RECORD_AUDIO. I'll not put it on the positive-list until you decided whether you really need it. What about SYSTEM_ALERT_WINDOW? Do you overlay some controls or such?

courville commented 7 months ago

I vaguely remember that it was used for the floating player when in overlay mode but I need to check that this is still the case. I do believe that the permission is not auto-granted anyway and needs to be granted in the Android app permission setting right? Let me try to remove it and tell you how it goes. Err I will have to see how it goes on API21 too... pffff.

IzzySoft commented 7 months ago

That (floating something) matches my guess. And yes, IIRC it should be a runtime permission, though it was already added with SDK19 (Kitkat) – but it seems under some conditions, it is granted automatically (e.g. WhatsApp does on incoming calls). But usually an app must request it, yes.

But if you assure me it's used and needed for that, I'll believe you (it totally makes sense, you've convinced me) and add it to the allow-list, so it won't pop up with future scans here.

IzzySoft commented 7 months ago

OK, all scanner options are now enabled here, so new findings popped up:

android.permission.READ_MEDIA_IMAGES android.permission.READ_MEDIA_AUDIO android.permission.READ_MEDIA_VIDEO android.permission.READ_EXTERNAL_STORAGE

Still, apart from usesCleartextTraffic nothing has been added to the allow list here, waiting for your confirmation @courville. I guess the READ_MEDIA_* permissions can be added to the allow-list straight away as Nova is supposed to play local media, right? But as one needs at least Android 5 to run the app, could READ_EXTERNAL_STORAGE dropped then (the other permissions indicate you are already using SAF, so this one would not be needed IMHO)?

courville commented 7 months ago

@IzzySoft, you are correct, nova being a video player, it relies on Android Media Library to find and index files on local storages including USB drives. In order to get the list of video files and possibly associated posters, nova thus requests READ_MEDIA_(VIDEO|IMAGES). I have added READ_MEDIA_AUDIO in the mix that I could remove.

To my knowledge READ_EXTERNAL_STORAGE is used for old and recent Android versions (nova targets API 21), see https://github.com/nova-video-player/aos-AVP/issues/336 and more specifically this part:

MANAGE_EXTERNAL_STORAGE is supported for API>=30, needs to be granted by user on API>=31 via android.settings.MANAGE_APP_ALL_FILES_ACCESS_PERMISSION that does not exist on Android TV until API>=34
WRITE_EXTERNAL_STORAGE is maxSdkVersion 29, auto-granted API<=22, requires explicit user permission 23<=API<=32, i.e. not required to be in manifest 30<=API<=32 but programmatically requested
READ_EXTERNAL_STORAGE is maxSdkVersion 32, auto-granted API<=22, requires explicit user permission 23<=API<=32
for API>=33 instead of using WRITE_EXTERNAL_STORAGE and READ_EXTERNAL_STORAGE, more granularity is required and READMEDIA(VIDEO|AUDIO|IMAGE) need to be requested when MANAGE_EXTERNAL_STORAGE is not used
for API>=33, POST_NOTIFICATIONS permission is also required

IzzySoft commented 7 months ago

Oops – another notification that didn't reach me. Apologies for the late response – and thanks for yours!

I've just added the READ_MEDIA_* permissions to Nova's allow-list. As videos include images and audio, maybe you plan to support "videos without images" (i.e. "playing plain audio") as well – like e.g. VLC and some other media players do? So shall I put that as explanation then?

*_EXTERNAL_STORAGE: those are the 2 permissions with the most complex rules as I just learned. Not only depending on the Android version on the device, but also on an app's targetSdkVersion plus (IIRC) in some cases minSdkVersion, and then of course different depending on what kind of files you want to access :see_no_evil: Added that to the allow-list as well – to not confuse those who don't develop apps themselves, simply stating "still needed on some Android versions to access media files" :man_shrugging:

That leaves MANAGE_EXTERNAL_STORAGE then. Err… Nova doesn't even request that, so I guess it was "for completeness" on the storage weirdness?

That said: any progress concerning RECORD_AUDIO, before we forget this?

Current settings:

      android.permission.READ_EXTERNAL_STORAGE: still needed on some Android versions to access media files
      android.permission.READ_MEDIA_AUDIO: needed to access those media files
      android.permission.READ_MEDIA_IMAGES: needed to show associated images (e.g. video thumbnails)
      android.permission.READ_MEDIA_VIDEO: needed to play local videos
      android.permission.SYSTEM_ALERT_WINDOW: for the "floating player"

Those explanations are now also displayed next to their corresponding permissions on Nova's details page in my repo – for those who check such things before installing apps :wink:

courville commented 7 months ago

@IzzySoft, MANAGE_EXTERNAL_STORAGE permission is regulated by Google and though nova requested it for some good reasons, it has been denied by Google.

IzzySoft commented 7 months ago

nova requested it for some good reasons

Ah! Maybe you could name those good reasons, or at least some of them? Then I'd add the permission to the allow list of Nova together with those reasons, and consider this solved.

courville commented 7 months ago

This is there https://home.courville.org/nova_video_player-faq/index.html

For good reasons Google restricts MANAGE_EXTERNAL_STORAGE permission since API31. Nova thus switched to MediaStore API. As a consequence nova is only able to see files registered as Media files by Google (missing video/subtitles formats such as ASS, NFO, torrent files). These files are not modifiable nor visible from nova when using local storage (incl. USB HDDs). This creates loss of functionality & incomprehension from nova users. Numerous appeals were issued & proper request filed for being granted the permission with explanation video. Only got default "no answers"/"not compliant" without having a Google support taking time to understand/review the case properly. Note that many other video players have the wanted MANAGE_EXTERNAL_STORAGE permission: e.g. VLC, mx player, video player all format, Video Player KMP, kodi... I consider this as unfair treatment and discrimination. Sad that an app that has more than 500k active users and 2M downloads on Google Play cannot get proper attention from Google.

However I do not want to make a special release that has MANAGE_EXTERNAL_STORAGE for github: I want it to be the same that I have on google play in order to not fragment the validation base.

IzzySoft commented 6 months ago

! repo/org.courville.nova_2588111.apk declares sensitive permission(s): android.permission.RECORD_AUDIO

You are not intentionally leaving my question on this one unanswered, are you? :eyes:

courville commented 6 months ago

@IzzySoft, sorry I thought I answered this one and I confirm that it is required for voice search.

IzzySoft commented 6 months ago

Thanks, added. Just out of curiosity: you're not using the system's SpeechToText there? Not complaining, as that might not be available on every device (and knowing the keyword, I just found it mentioned in the FAQ, thanks!)

courville commented 6 months ago

I will have a look but far down my priority list for now sorry. I will ping if status updates.

IzzySoft commented 6 months ago

That's fine of course. It's being made transparent, the reason is clear. If it can be done without (and without compromising usability) all the better – if not, leave it as-is. Thanks!

IzzySoft commented 3 months ago

SYSTEM_ALERT_WINDOW seems to be gone with the last update, cool! The floating player still works, though? And btw:

! repo/org.courville.nova_2596673.apk contains signature block blobs: 0x504b4453 (DEPENDENCY_INFO_BLOCK; GOOGLE)

can easily be avoided:

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.

courville commented 3 weeks ago

Apologies for bringing each thing up separately, but I'm extending those checks slowly to not get swamped by too many reports. Here's today's alert:
! repo/org.courville.nova_2585577.apk declares risky permissions: android.permission.SYSTEM_ALERT_WINDOW android.permission.RECORD_AUDIO
We already addresses RECORD_AUDIO. I'll not put it on the positive-list until you decided whether you really need it. What about SYSTEM_ALERT_WINDOW? Do you overlay some controls or such?

Cf. https://github.com/nova-video-player/aos-AVP/issues/1252, SYSTEM_ALERT_WINDOW is back to avoid regression.

IzzySoft commented 3 weeks ago

Ah, PiP – perfectly makes sense. Like floating windows. Will add it to the green list once it shows up, thanks!

Btw, will you address that DEPENDENCY_INFO_BLOCK?

courville commented 2 weeks ago

Ah, PiP – perfectly makes sense. Like floating windows. Will add it to the green list once it shows up, thanks!

Btw, will you address that DEPENDENCY_INFO_BLOCK?

I think I did address the DEPENDENCY_INFO_BLOCK already cf. 847a2d92c01270e2164df64fad3dc99190d58873 in aos-Video (it applies only in the universal multiarch build).

IzzySoft commented 2 weeks ago

It's still there with versionCode 2600896 (version 6.2.90-20240707.1829).

it applies only in the universal multiarch build

And why not in the others? The universal is too big for IzzyOnDroid, which uses the armeabi one currently – and even that is already pretty close to the per-app size limit of 30 MB :wink:

Btw, I also miss the IzzyOnDroid badge and shield, maybe you want to pick them and add them to the Readme?

courville commented 1 week ago

@IzzySoft, I have just added the IzzyOnDroid badges.

IzzySoft commented 1 week ago

Nice, thanks! And both at that :hugs:

nova-video-player / aos-AVP

Why is usesCleartextTraffic set? #1042