Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

What for is READ_EXTERNAL_STORAGE needed? #1309

Closed
IzzySoft opened this issue Jan 20, 2024 · 29 comments
Closed

What for is READ_EXTERNAL_STORAGE needed? #1309

IzzySoft opened this issue Jan 20, 2024 · 29 comments
Labels
bug Something isn't working

Comments

@IzzySoft
Copy link

IzzySoft commented Jan 20, 2024

My scanner just got additional checks implemented, and promptly reported for today's update:

! repo/dev.imranr.obtainium_1248.apk declares risky permission(s): android.permission.READ_EXTERNAL_STORAGE android.permission.REQUEST_DELETE_PACKAGES

Minimum Android version to use your app is set to Android 7. So shouldn't storage access rather be handled by SAF (Storage Access Framework)? Is there anything requiring READ_EXTERNAL_STORAGE instead?

As for REQUEST_DELETE_PACKAGES: app description only states "allows you to install and update". I assume one can also delete apps from within Obtainium then?

Thanks in advance for clarification!

@IzzySoft
Copy link
Author

PS: I see there are APKs available now for a separate F-Droid build flavor. I guess the only difference is the self-updater being removed for those? Because then I'd switch to that flavor for my repo, where self-updater are also violating the inclusion criteria (same reason as for F-Droid).

@ImranR98
Copy link
Owner

ImranR98 commented Jan 20, 2024

I switched to a different APK installer plugin which indicated it needs that permission: https://github.com/playbott/android_package_installer (8b123ac#diff-8b7e9df87668ffa6a04b32e1769a33434999e54ae081c52e5d943c541d4c0d25)

That said, I removed the permission and APKs still install fine on Android 14, so it's possible that permission is only needed for older versions (the plugin supports Android 5) or only for APKs that are outside Obtainium's own cache folder. I'll test more and remove the permission in the next release if possible.

The F-Droid variant of Obtainium also has a different App ID.

ImranR98 added a commit that referenced this issue Jan 20, 2024
Removed unnecessary READ_EXTERNAL_STORAGE permission (#1309)
@ImranR98
Copy link
Owner

Confirmed - removing the permission doesn't affect APK installs on Android 7 or 14.

@ImranR98 ImranR98 added bug Something isn't working TODO Issue to focus on for the next release labels Jan 20, 2024
@ImranR98
Copy link
Owner

@ImranR98 ImranR98 removed the TODO Issue to focus on for the next release label Jan 20, 2024
@IzzySoft
Copy link
Author

Thanks! I've just pulled that, and here's what my updater reports:

! repo/dev.imranr.obtainium_1249.apk declares risky permission(s): android.permission.REQUEST_DELETE_PACKAGES android.permission.READ_EXTERNAL_STORAGE

So it's still there. And can you confirm that REQUEST_DELETE_PACKAGES is requested because Obtainium allows the user to uninstall apps (as the description doesn't mention it, see above)?

The F-Droid variant of Obtainium also has a different App ID.

Yes, that's how I noticed. My updater had pulled that one instead (as it only checked for the ABI, as I told it to).

@ImranR98
Copy link
Owner

That's weird, the string READ_EXTERNAL_STORAGE doesn't exist anywhere else in the repo - not sure where it's coming from.

For REQUEST_DELETE_PACKAGES, yes you can uninstall apps from within Obtainium too.

@ImranR98 ImranR98 reopened this Jan 21, 2024
@DwainZwerg
Copy link
Contributor

@IzzySoft Ich schreibe dir mal auf Deutsch, weil ich weiß, dass du deutscher bist und auch im Shiftforum unterwegs (bin hier einer der Übersetzer ins Deutsche …): Zum Deinstallieren der Apps: Markiere eine App, drücke auf den 🗑 und wähle zusätzlich den Wechselschalter „Vom Gerät deinstallieren“. Wenn du dann auf „weiter“ klicken würdest, würde die App deinstalliert werden.

@IzzySoft
Copy link
Author

@DwainZwerg Danke! Ich verwende Obtainium selbst nicht, sonst hätte ich ja einfach nachgeschaut. Ist halt in meinem Repo, und in dem gibt es seit ein paar Tagen halt zusätzliche APK-Prüfungen. Kamen schon einige (zum Glück kleinere) Dinge hoch, die zu Verbesserungen einiger Apps führten 😃

Thanks to @ImranR98 as well! Will add REQUEST_DELETE_PACKAGES to Obtainium's allow-list then.

That's weird, the string READ_EXTERNAL_STORAGE doesn't exist anywhere else in the repo - not sure where it's coming from.

Going by your AndroidManifest.xml I see WRITE_EXTERNAL_STORAGE, which alone would lead to Android implicitly grant READ_EXTERNAL_STORAGE – but then my scanner would mark it as "implicit permission" which it did not. So something else must have brought it in explicitly. Does Obtainium really need those storage permissions? Else maybe removing the latter will get you rid of the former as well; could be some "automation" taking the implicit grant ahead as "it is always better to request grants explicitly".

But maybe you couldn't find it anymore because you removed it with #1311 ? 😜 Guess that means this issue is solved then, so I'll close it now. Should something pop up with the next release (which I do not expect it to), I can still reopen.

Thanks to all of you for your help!

@IzzySoft
Copy link
Author

Oh, PS: Are you OK with me switching to the .fdroid variant in my repo to ensure folks using both, my repo and Obtainium, always get the APK with my additional scans (which are not performed at F-Droid)? That would make it a "double-listed" app then, though. I usually remove apps from my repo once they hit F-Droid. So shall I still keep Obtainium in at all? As my updater with its current setup would randomly pick one of the two builds (it just looks for the ABI, and now each ABI has 2 APKs at releases), I need to take an action anyway: either switch to the .fdroid build, or tell the updater to ignore that.

@ImranR98
Copy link
Owner

I'm not sure what the best path forward is for now, since the F-Droid build isn't actually being released on F-Droid right now - the process is mainly being pushed forward by other contributors (personally I was not interested in it) and they seem to still be figuring out whether reproducible builds will work, etc. So probably best to stick with the non-.fdroid APKs for now.

@IzzySoft
Copy link
Author

Yes, I see: if it ends up non-RB, it might cause confusion. Though self-updater have the same criteria with my repo as they have with F-Droid, it seems a bit weird asking the authors of an app intended to update all other apps to exclude itself from that.

I know Obtainium covers a lot of sources, including F-Droid and my repo. Do you count all sources as "equal"? Or does Obtainium keep a kind of priority list ("take it from A if possible, else try B, then C…") – and if so, is the weighting somehow transparent? Idea behind my question: if the self-updater would put the two mentioned repos first, there'd just be a minor gap (less than 24) for my repo when you release a new version (some days for F-Droid however). Or maybe it could take the install-source in mind and try that first, only switching to another if it failed for a day or so. Then the reason behind the self-updater policy would be gone (at least for me, I cannot speak for F-Droid there).

That said, I've pinned the updater to the non-fdroid variant now and re-enabled the daily update check (had stalled updates until we decided on a variant). Once Obtainium becomes available at F-Droid we should be sure how to proceed. That's usually when I remove an app from my repo (after a decent overlap), so the question could become moot then.

So currently, the only remaining warning (I just manually triggered 1249 to be pulled) is

! repo/dev.imranr.obtainium_1249.apk declares risky permission(s): android.permission.READ_EXTERNAL_STORAGE

The others have all been solved. Funny, this issue is closed with all things addressed – except for the permission it was opened for 🤪

@ImranR98
Copy link
Owner

"take it from A if possible, else try B, then C…"

Apps can only be added from one source, so if you add an app X from GitHub, you can't add it again from APKPure (for example). So there's no fallback logic or priority order.

this issue is closed with all things addressed – except for the permission it was opened for 🤪

I can reopen it to keep tracking that (it's weird, I don't see where that permission is still coming from).

@IzzySoft
Copy link
Author

@ImranR98 unfortunately, READ_EXTERNAL_STORAGE is still being reported with today's release:

! repo/dev.imranr.obtainium_1250.apk declares risky permission(s): android.permission.READ_EXTERNAL_STORAGE

It has no asterisk appended, so it must be declared explicitly somewhere. I can confirm I didn't find it mentioned anywhere but in this issue, the PR referenced above, and the commits from that PR, which suggests some dependency must bring it in so it gets merged into the resulting/final AndroidManifest.xml. Could you maybe check in your build tree, which should have all dependencies included? I'm not familiar with Flutter, so I'd not even know where to check with the Dart code – but if you'd ask me for possible candidates, path_provider sounds like one… Bingo: it mentions it here – though as tools:node="remove":

<uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE"
    tools:node="remove" />

Maybe putting that into your own AndroidManifest.xml would solve it for the final one? The comment there suggests that this would also get rid of it being "implied" (as you declare WRITE_EXTERNAL_STORAGE here, READ_INTERNAL_STORAGE would become an "implied permission" even if you got rid of it elsewhere – unless that permission is not needed either).

I've checked the other modules with names sounding like possible candidates, but could not find the permission mentioned there.

@ImranR98 ImranR98 reopened this Jan 28, 2024
@ImranR98 ImranR98 added the TODO Issue to focus on for the next release label Jan 28, 2024
ImranR98 added a commit that referenced this issue Jan 28, 2024
Better support for SourceForge (#1352), Remove READ_EXTERNAL_STORAGE again (#1309) + fix typo
@ImranR98
Copy link
Owner

ImranR98 commented Feb 3, 2024

Should hopefully be gone now: https://github.com/ImranR98/Obtainium/releases/tag/v1.0.2

@ImranR98 ImranR98 closed this as completed Feb 3, 2024
@ImranR98 ImranR98 removed the TODO Issue to focus on for the next release label Feb 3, 2024
@IzzySoft
Copy link
Author

IzzySoft commented Feb 3, 2024

Unfortunately not:

$ iod repo get dev.imranr.obtainium
dev.imranr.obtainium: looking for 'https://api.github.com/repos/ImranR98/Obtainium/releases'
dev.imranr.obtainium: checking tag 'v1.0.2'
dev.imranr.obtainium: lastRelNo set to '1.0.2', checking for files
dev.imranr.obtainium: Upstream file date (2024-02-03 10:40) is newer than ours (2024-01-28 19:39).
dev.imranr.obtainium: returning ['1.0.2','https://github.com/ImranR98/Obtainium/releases/download/v1.0.2/app-armeabi-v7a-release.apk',1706953219]
dev.imranr.obtainium: 1.0.1/1.0.2, https://github.com/ImranR98/Obtainium/releases: https://github.com/ImranR98/Obtainium/releases/download/v1.0.2/app-armeabi-v7a-release.apk
- Grabbing update for dev.imranr.obtainium: OK
- Checking 'repo/dev.imranr.obtainium_2522.apk' for libraries and malware …
- Checking the app's AndroidManifest.xml …
! repo/dev.imranr.obtainium_2522.apk declares sensitive permission(s): android.permission.READ_EXTERNAL_STORAGE*
dev.imranr.obtainium: check if repo contains FUNDING.yml
dev.imranr.obtainium: looking for 'https://api.github.com/repos/ImranR98/Obtainium/contents/.github'
dev.imranr.obtainium: looking for 'https://api.github.com/repos/ImranR98/Obtainium/contents/'
dev.imranr.obtainium: looking for 'https://api.github.com/repos/ImranR98/.github/contents/'
dev.imranr.obtainium: Github reports "Not Found" for https://api.github.com/repos/ImranR98/.github/contents/
dev.imranr.obtainium: no FUNDING.yml detected.
dev.imranr.obtainium: no Fastlane configured, skipping Fastlane check.

no Fastlane configured

I see you have just added that, wonderful! So I've checked and configured that on my end now and manually triggered a fetch (in the future, this will happen automatically whenever an update is pulled):

$ iod repo fastlane dev.imranr.obtainium
dev.imranr.obtainium: check if repo contains FUNDING.yml
dev.imranr.obtainium: looking for 'https://api.github.com/repos/ImranR98/Obtainium/contents/.github'
dev.imranr.obtainium: looking for 'https://api.github.com/repos/ImranR98/Obtainium/contents/'
dev.imranr.obtainium: looking for 'https://api.github.com/repos/ImranR98/.github/contents/'
dev.imranr.obtainium: Github reports "Not Found" for https://api.github.com/repos/ImranR98/.github/contents/
dev.imranr.obtainium: no FUNDING.yml detected.
dev.imranr.obtainium: calling 'getFastlaneMeta(github,[host:github.com,owner:ImranR98,repo:Obtainium,path:/fastlane/metadata/android])'
dev.imranr.obtainium: FastlaneFeatures title,shortdesc,fulldesc,icon,featureGraphic,screenshots
dev.imranr.obtainium: looking for 'https://api.github.com/repos/ImranR98/Obtainium/contents/fastlane%2Fmetadata%2Fandroid'
dev.imranr.obtainium: looking for 'https://api.github.com/repos/ImranR98/Obtainium/contents/fastlane%2Fmetadata%2Fandroid%2Fen-US'
dev.imranr.obtainium: looking for 'https://api.github.com/repos/ImranR98/Obtainium/contents/fastlane%2Fmetadata%2Fandroid%2Fen-US%2Fimages'
dev.imranr.obtainium: looking for 'https://api.github.com/repos/ImranR98/Obtainium/contents/fastlane%2Fmetadata%2Fandroid%2Fen-US%2Fimages%2FphoneScreenshots'
dev.imranr.obtainium: checking locale 'en-US'
dev.imranr.obtainium: updating 'metadata/dev.imranr.obtainium/en-US/full_description.txt'
dev.imranr.obtainium: updating 'metadata/dev.imranr.obtainium/en-US/short_description.txt'
dev.imranr.obtainium: updating 'metadata/dev.imranr.obtainium/en-US/title.txt'
dev.imranr.obtainium: updating 'repo/dev.imranr.obtainium/en-US/featureGraphic.png'
dev.imranr.obtainium: updating 'repo/dev.imranr.obtainium/en-US/icon.png'
dev.imranr.obtainium: updating 'repo/dev.imranr.obtainium/en-US/phoneScreenshots/1.png'
dev.imranr.obtainium: updating 'repo/dev.imranr.obtainium/en-US/phoneScreenshots/2.png'
dev.imranr.obtainium: updating 'repo/dev.imranr.obtainium/en-US/phoneScreenshots/3.png'
dev.imranr.obtainium: updating 'repo/dev.imranr.obtainium/en-US/phoneScreenshots/4.png'
dev.imranr.obtainium: updating 'repo/dev.imranr.obtainium/en-US/phoneScreenshots/5.png'
dev.imranr.obtainium: updating 'repo/dev.imranr.obtainium/en-US/phoneScreenshots/6.png'
dev.imranr.obtainium: cross-checking for obsolete screenshots
dev.imranr.obtainium: screenshots in Fastlane: 1,2,3,4,5,6
dev.imranr.obtainium: removing 'repo/dev.imranr.obtainium/en-US/phoneScreenshots/1.apps.jpg'
dev.imranr.obtainium: removing 'repo/dev.imranr.obtainium/en-US/phoneScreenshots/2.dark_theme.jpg'
dev.imranr.obtainium: removing 'repo/dev.imranr.obtainium/en-US/phoneScreenshots/3.material_you.jpg'
dev.imranr.obtainium: removing 'repo/dev.imranr.obtainium/en-US/phoneScreenshots/4.app.jpg'
dev.imranr.obtainium: removing 'repo/dev.imranr.obtainium/en-US/phoneScreenshots/5.app_opts.jpg'
dev.imranr.obtainium: removing 'repo/dev.imranr.obtainium/en-US/phoneScreenshots/6.app_webview.jpg'
dev.imranr.obtainium: local screenshots checked: 1.apps,1,2.dark_theme,2,3.material_you,3,4.app,4,5.app_opts,5,6.app_webview,6

Thanks!

@ImranR98
Copy link
Owner

ImranR98 commented Feb 3, 2024

Unfortunately not

Huh, I wonder where it's coming from.

@IzzySoft
Copy link
Author

IzzySoft commented Feb 4, 2024

I have no clue. Especially as you use tools:node="remove" in the "main manifest" – I've been told that should do it and overrule whatever other dependencies bring in. Let me take a closer look before I cause you more work. It might well be my scanner ignoring the attribute…

Oof. Even more strange. aapt seems to ignore that:

uses-implied-permission: name='android.permission.READ_EXTERNAL_STORAGE' maxSdkVersion='29' reason='requested WRITE_EXTERNAL_STORAGE'

using androguard axml dev.imranr.obtainium_2522.apk I cannot see the declaration either, not with nor without the tools:node declaration. So it doesn't seem to make it into the AXML. Maybe tools:node="remove" does not work for implied permissions? Then I could just add a proper reason to the allow-list stating that, no matter what, Android implies that automatically – and we'd need to add a reason to WRITE_EXTERNAL_STORAGE.

Though that's only needed with my repo (F-Droid does not perform such "extra screening"). I was notified Obtainium is now available on F-Droid (congrats!), albeit with a different package name. Do you want me to keep it in my repo nevertheless? And if so, would you be OK if I'd keep only its latest version available there (currently I keep 2, but that sums up to 40 MB which is slightly over the per-app size limit of 30 MB)?

@DJCrashdummy
Copy link

DJCrashdummy commented Feb 6, 2024

@ImranR98 unfortunately since Obtainium 1.0.2 importing via Obtainium-Inport or URLs from a file (e.g. OPML) stopped working and i'm pretty sure it has to do with this issue resp. the permissions.
-> i granted all permissions (even to a folder for exporting) but i only get an error message when trying the Obtainium-Inport ("PlatformException(read_external_storage_denied, User did not allow reading external storage, null, null)") and when pressing the URLs from a file (e.g. OPML) button simply nothing happens.

my workaround was downgrading to Obtainium 1.0.1 and using the Obtainium-Import button which worked like a charm.
...interestingly Obtainium-Export (automatically) still works in 1.0.2!


[...] Obtainium is now available on F-Droid (congrats!), albeit with a different package name. Do you want me to keep it in my repo nevertheless?

@IzzySoft i would vote to rather keep it in your repo as it is technically a different app (like it is done with KeePassDX)... and to keep it easy to get the self-updating version of Obtainium (since this is not possible with the F-Droid version).

@ImranR98 ImranR98 reopened this Feb 6, 2024
@ImranR98 ImranR98 added the TODO Issue to focus on for the next release label Feb 6, 2024
@shuvashish76
Copy link

shuvashish76 commented Feb 6, 2024

To keep it easy to get the self-updating version of Obtainium.

Specifically "Self-updaters" /downloaders aren't allowed on F-Droid, see their inclusion policy.

Details

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.

Update checkers are tolerated with with certain ARs such as Tracking.

Details

  • Checking for updates without your knowledge or permission.
    Examples of where it would not be applied - any of the above, if the functionality is opt-in (i.e. you are asked before it happens) and disabled by default.

As per my knowledge Izzy repo follows F-Droid standard criterias so all those are applicable to izzyrepo as well.


Only difference I see in both is that Obtanium isn't added to the App list by default on F-Droid. "Check for updates on opening App details page" is turned on in both.

  1. It makes sense, not to add Obtanium to the list by default for F-Droid. ✔️
  2. As per the criteria Tracking AF should be applicable for Izzy but exception can be made as
  • i) Downloader: It's an app store so what do you expect ⸮
  • ii) Update checker: It doesn't check updates on start up but only when you click on App details page for Obtanium which is added by default.

What's @IzzySoft your opinion on it?

like it is done with KeePassDX

@ImranR98 See #1378

@IzzySoft
Copy link
Author

IzzySoft commented Feb 6, 2024

@DJCrashdummy

i would vote to rather keep it in your repo as it is technically a different app (like it is done with KeePassDX)

If @ImranR98 says so, it can be done and I will add a proper note to the YAML metadata here. Though I still don't know the differences between the two – apart from the package name. Or I knew them once and simply forgot 🙈

@shuvashish76

Specifically "Self-updaters" /downloaders aren't allowed on F-Droid

Same policy here. But going by the letter, that would mean the F-Droid client itself as well as any 3rd party F-Droid clients were not allowed either. A package manager has the purpose of managing packages. So apps where this is clearly the main purpose are exempted.

As per my knowledge Izzy repo follows F-Droid standard criterias so all those are applicable to izzyrepo as well.

Almost 😉 But yes, the relevant part is almost identical:

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 to get back to the F-Droid clients and, more on-topic, to Obtainium: Why did you install Obtainium? To let it update all your apps (as that is what it does). How did you know it does that? By its app description. Does the app description point out where it installs apps from? Yes. So did you install Obtainium? If yes, it's one of your apps – and you've made the explicit and informed decision to have it update your apps – including itself.

As per the criteria Tracking AF should be applicable for Izzy

Huh? Because it tracks available app updates? 🙈 No. But I guess that is answered now, right?

@shuvashish76
Copy link

shuvashish76 commented Feb 6, 2024

A package manager has the purpose of managing packages. So apps where this is clearly the main purpose are exempted.

Thanks 👍🏿. @ImranR98 Instead of an empty App list you could add Obtanium for F-Droid version & point the source to F-Droid link & of course GitHub for Izzy version.

@IzzySoft
Copy link
Author

IzzySoft commented Feb 6, 2024

If it shall be kept in my repo, you can pick a badge for that as well (one directly behind that link, more in the assets/ directory linked from there) 😃

@ImranR98
Copy link
Owner

unfortunately since Obtainium 1.0.2 importing via Obtainium-Inport or URLs from a file (e.g. OPML) stopped working and i'm pretty sure it has to do with this issue resp. the permissions.

@DJCrashdummy it works on my end (tested on Android 7 and 14 emulators, and a GrapheneOS Android 14 phone). Could you create a new issue w/ your OS details? Also try reinstalling the app.

@IzzySoft the only differences between the FDroid and regular versions is the App ID and the fact that the regular one contains an 'Obtainium' entry pre-added. I don't see why you couldn't keep the regular one on IzzyDroid.

@ImranR98 ImranR98 removed the TODO Issue to focus on for the next release label Feb 10, 2024
@IzzySoft
Copy link
Author

the only differences between the FDroid and regular versions is the App ID and the fact that the regular one contains an 'Obtainium' entry pre-added. I don't see why you couldn't keep the regular one on IzzyDroid.

Thanks! Added a corresponding note to the app's metadata to keep it.

@ImranR98
Copy link
Owner

Just FYI I'm going to remove the tools:node="remove" since it seems to have broken some functionality on Android 12 (#1391).

@IzzySoft
Copy link
Author

OK, understood. So the explanation then would be "needed for the import functionality"?

@ImranR98 ImranR98 reopened this Feb 19, 2024
@ImranR98
Copy link
Owner

Yep sounds right

@ImranR98 ImranR98 closed this as not planned Won't fix, can't repro, duplicate, stale Feb 19, 2024
@IzzySoft
Copy link
Author

OK, prepared that. Thanks!

@bornasalman
Copy link

Yep sounds right

Hey, what was the outcome of this? Just reformatted my unrooted S23, did some ADB tweaks (none app related, all system level), and now the Export is greyed out and the Import loops back to the main app page.

No logcat or app debug logs are outputted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants