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

Some Directory getFile methods are redudant or not safe at various supported API levels #1091

Closed
Zardozz opened this issue Aug 2, 2022 · 1 comment

Comments

@Zardozz
Copy link
Contributor

Zardozz commented Aug 2, 2022

Describe the bug
The EXTERNAL_FILES version of getFile method (https://github.com/ACRA/acra/blob/master/acra-core/src/main/java/org/acra/file/Directory.kt#L51 ) is redundant
OR
It will should generate a Null pointer exception as getExternalFilesDir will return null if used when ACRA is running from the minimum supported API (14) to API 18 because as per docs

Starting in Build.VERSION_CODES.KITKAT, no permissions are required to read or write to the returned path; it's always accessible to the calling app.

The NPE can be avoided by adding a suitable Manifest entry to the core library.

e.g. to https://github.com/ACRA/acra/blob/master/acra-core/src/main/AndroidManifest.xml add

<uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" android:maxSdkVersion="18"/>

Also should really check that for any off the "EXTERNAL" methods storage is mounted as per "EXTERNAL" method in Context Docs
i.e.

Shared storage may not always be available, since removable media can be ejected by the user. Media state can be checked using Environment#getExternalStorageState(File).

Also Directory.kt uses getExternalStorageDirectory and

Writing to this path requires the Manifest.permission.WRITE_EXTERNAL_STORAGE permission, and starting in Build.VERSION_CODES.KITKAT, read access requires the Manifest.permission.READ_EXTERNAL_STORAGE permission, which is automatically granted if you hold the write permission.

I've not found any direct usages, though it could probably be used in one of the optional config paths.

Expected behaviour
Not to provide methods that won't work correctly at various supported by the library API versions or be configured with the right permission and state tests for them to work correct.

I would suggest that "EXTERNAL_FILES", "EXTERNAL_CACHE" and "EXTERNAL_STORAGE" variants of getFile are removed as the simplest solution.

Version

  • Android: API 14 onwards
  • ACRA 5.9.5
@F43nd1r
Copy link
Member

F43nd1r commented Oct 11, 2022

These fields are used for configuration. A user might want to e.g. add attachments from external storage, that is a valid use case. It is however the users responsibility to make sure permissions are available. I have added a note to the documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants