diff --git a/.github/workflows/android.yml b/.github/workflows/android.yml index 33bdc60..5dd9d21 100644 --- a/.github/workflows/android.yml +++ b/.github/workflows/android.yml @@ -82,13 +82,19 @@ jobs: run: tools/ci/vendor-libiconv.sh - name: Stage OpenSSL statics into prebuild/ run: tools/ci/fetch-openssl-statics.sh # picks up $OPENSSL_ANDROID_ROOT from the image - - name: Gradle assemble + lint - run: ./gradlew --no-daemon assembleDebug assembleRelease lint + - name: Gradle assemble + test + lint + run: ./gradlew --no-daemon assembleDebug assembleRelease testDebugUnitTest lint - name: Check 16 KB .so alignment run: | for apk in app/build/outputs/apk/*/*.apk; do tools/ci/check-so-alignment.sh "$apk" done + - uses: actions/upload-artifact@v4 + if: always() # the report is what says which case broke + with: + name: test-results + path: app/build/reports/tests/** + if-no-files-found: warn - uses: actions/upload-artifact@v4 with: name: apk diff --git a/app/build.gradle b/app/build.gradle index 8b33630..1e6adf2 100755 --- a/app/build.gradle +++ b/app/build.gradle @@ -58,4 +58,6 @@ dependencies { implementation 'androidx.core:core:1.13.1' implementation 'androidx.fragment:fragment:1.8.2' + + testImplementation 'junit:junit:4.13.2' } diff --git a/app/src/main/AndroidManifest.xml b/app/src/main/AndroidManifest.xml index f4afb1c..f55a1a8 100755 --- a/app/src/main/AndroidManifest.xml +++ b/app/src/main/AndroidManifest.xml @@ -2,9 +2,6 @@ - - - diff --git a/app/src/main/java/com/httrack/android/FileChooserActivity.java b/app/src/main/java/com/httrack/android/FileChooserActivity.java index a1bad4e..2583289 100755 --- a/app/src/main/java/com/httrack/android/FileChooserActivity.java +++ b/app/src/main/java/com/httrack/android/FileChooserActivity.java @@ -27,7 +27,6 @@ public class FileChooserActivity extends Activity implements protected File projectRootFile; protected File defaultRootFile; - protected File sdcardRootFile; protected final List> files = new ArrayList>(); @@ -144,8 +143,6 @@ protected void onCreate(final Bundle savedInstanceState) { .get("com.httrack.android.rootFile")); defaultRootFile = File.class.cast(extras .get("com.httrack.android.defaultHTTrackPath")); - sdcardRootFile = File.class.cast(extras - .get("com.httrack.android.sdcardHTTrackPath")); if (projectRootFile == null || defaultRootFile == null) { throw new RuntimeException("internal error"); } diff --git a/app/src/main/java/com/httrack/android/HTTrackActivity.java b/app/src/main/java/com/httrack/android/HTTrackActivity.java index e553804..d5b51bc 100755 --- a/app/src/main/java/com/httrack/android/HTTrackActivity.java +++ b/app/src/main/java/com/httrack/android/HTTrackActivity.java @@ -226,50 +226,23 @@ protected static synchronized void clearRunningInstance(final File profile) { } } - /* Get the SDCard directory, or @c null upon error (ie. no sdcard). */ - private File getSDCardStorage() { - final String state = Environment.getExternalStorageState(); - if (Environment.MEDIA_MOUNTED.equals(state)) { - return Environment.getExternalStorageDirectory(); - } else { - return null; - } - } - - /* Get the root storage: SDCard, ExternalStoragePublicDirectory, ExternalStorageDirectory, or fallback to FilesDir */ - private File getExternalStorage() { - final File f = getSDCardStorage(); - if (f != null) { - return f; - } else { - if (android.os.Build.VERSION.SDK_INT >= VERSION_CODES.FROYO) { - return Environment - .getExternalStoragePublicDirectory(Environment.DIRECTORY_DOWNLOADS); - } else { - // Fallback. - return getFilesDir(); - } - } - } - - private File getHTTrackDirectoryFromBaseDirectory(final File rootPath) { - // Naming - final File httrackBasePath = new File(rootPath, "HTTrack"); - final File httrackPath = new File(httrackBasePath, "Websites"); - - return httrackPath; - } - - /* Get the default root external storage. */ + /* + * The only place mirrors may live under scoped storage: our own external directory, which + * needs no permission at any target. The engine takes a POSIX path either way. + */ private File getDefaultHTTrackPath() { - return getHTTrackDirectoryFromBaseDirectory(getExternalStorage()); + final File external = getExternalFilesDir(null); + // Null while the volume is unmounted; internal storage keeps the engine writable. + return new File(external != null ? external : getFilesDir(), "Websites"); } - /* Get the alternate root external storage. */ - private File getSDCardHTTrackPath() { - final File rootPath = getSDCardStorage(); - - return rootPath != null ? getHTTrackDirectoryFromBaseDirectory(rootPath) : null; + /* + * Guards the persisted BasePath, which may name a public directory from an older build: + * from API 30 on, mkdirs() there merely fails without saying why. Null means undecided, + * never refused; see StoragePaths.isWritable. + */ + private Boolean isWritableProjectPath(final File path) { + return StoragePaths.isWritable(path, getExternalFilesDir(null), getFilesDir()); } /* @@ -278,7 +251,21 @@ private File getSDCardHTTrackPath() { private void computeStorageTarget() { final SharedPreferences settings = getSharedPreferences(PREFS_NAME, 0); final String base = settings.getString(BASE_NAME, null); - final File baseFile = base != null ? new File(base) : null; + File baseFile = base != null ? new File(base) : null; + + if (baseFile != null) { + final Boolean writable = isWritableProjectPath(baseFile); + if (Boolean.FALSE.equals(writable)) { + // Known foreign: the mirrors it names stay on disk, out of reach until imported. + Log.i(getClass().getSimpleName(), "dropping unusable base path " + base); + settings.edit().remove(BASE_NAME).apply(); + baseFile = null; + } else if (writable == null) { + // Undecided: fall back for this run, but keep the setting for when the volume returns. + Log.i(getClass().getSimpleName(), "cannot vet base path yet: " + base); + baseFile = null; + } + } if (baseFile != null && !baseFile.exists() && !baseFile.mkdirs()) { showNotification(getString(R.string.directory_does_not_exist) + ": " + base); @@ -307,6 +294,11 @@ private void computeStorageTarget() { * Set the base path. */ private void setBasePath(final String path) { + // Only a decided yes: an unvettable path must not be persisted. + if (!Boolean.TRUE.equals(isWritableProjectPath(new File(path)))) { + showNotification(getString(R.string.directory_does_not_exist) + ": " + path); + return; + } final SharedPreferences settings = getSharedPreferences(PREFS_NAME, 0); final SharedPreferences.Editor editor = settings.edit(); editor.putString(BASE_NAME, path); @@ -414,29 +406,9 @@ protected void ensureInternetIsAvailable() { } } - /* - * Ensure "hard" permissions are requested properly from user. - * This is a new Android 6 required step, sheesh. - * See - * and especially szedjani's insightful reply. - */ - private static final int REQUEST_WRITE_STORAGE = 1; private static final int REQUEST_INTERNET = 2; private static final int REQUEST_POST_NOTIFICATIONS = 3; - protected void ensureMediaIsAllowedHardPermissions() { - final boolean hasPermissionStorage = (ContextCompat.checkSelfPermission(this, - Manifest.permission.WRITE_EXTERNAL_STORAGE) == PackageManager.PERMISSION_GRANTED); - if (!hasPermissionStorage) { - Log.d("httrack", "Requesting access to storage"); - ActivityCompat.requestPermissions(this, - new String[]{Manifest.permission.WRITE_EXTERNAL_STORAGE}, - REQUEST_WRITE_STORAGE); - } else { - Log.d("httrack", "Access to storage is allowed"); - } - } - /* * Only a runtime permission from Android 13 on; below that the manifest entry is enough. * Targeting 33+ also drops the automatic prompt, so without this the app posts nothing. @@ -464,20 +436,12 @@ protected void ensureNotificationsAreAllowed() { } /* - * Callback to receive ensureMediaIsAllowedHardPermissions() permission feedback. + * Callback to receive permission feedback. */ @Override public void onRequestPermissionsResult(int requestCode, String[] permissions, int[] grantResults) { super.onRequestPermissionsResult(requestCode, permissions, grantResults); switch (requestCode) { - case REQUEST_WRITE_STORAGE: { - if (grantResults.length == 0 || grantResults[0] != PackageManager.PERMISSION_GRANTED) { - Toast.makeText(this, "The app was not allowed to write to your storage. Hence, it cannot function properly. Please consider granting it this permission", Toast.LENGTH_LONG).show(); - } else { - Log.d("httrack", "Permission to storage is granted"); - } - } - break; case REQUEST_INTERNET: { if (grantResults.length == 0 || grantResults[0] != PackageManager.PERMISSION_GRANTED) { Toast.makeText(this, "The app was not allowed to access the Internet. Hence, it cannot function properly. Please consider granting it this permission", Toast.LENGTH_LONG).show(); @@ -508,7 +472,6 @@ protected boolean ensureExternalStorage() { Log.d("httrack", "called ensureExternalStorage"); computeStorageTarget(); - ensureMediaIsAllowedHardPermissions(); ensureInternetIsAvailable(); final File root = getProjectRootFile(); @@ -2256,15 +2219,10 @@ public void onClickBasePath(final View view) { final Intent intent = new Intent(this, FileChooserActivity.class); fillExtra(intent); - final File defaultPath = getDefaultHTTrackPath(); + // Only our own directory is left to browse, so there is no second root to offer. intent.putExtra("com.httrack.android.defaultHTTrackPath", - defaultPath); + getDefaultHTTrackPath()); - final File f = getSDCardHTTrackPath(); - if (f != null && !f.equals(defaultPath)) { - intent.putExtra("com.httrack.android.sdcardHTTrackPath", - f); - } startActivityForResult(intent, ACTIVITY_FILE_CHOOSER); } diff --git a/app/src/main/java/com/httrack/android/StoragePaths.java b/app/src/main/java/com/httrack/android/StoragePaths.java new file mode 100644 index 0000000..fc24f5a --- /dev/null +++ b/app/src/main/java/com/httrack/android/StoragePaths.java @@ -0,0 +1,43 @@ +package com.httrack.android; + +import java.io.File; +import java.io.IOException; + +/** + * Where mirrors may be written. Pure path math, kept apart from the Context lookups so that it + * can be exercised off-device: this is the boundary that decides whether a persisted setting is + * honoured or destroyed. + */ +final class StoragePaths { + private StoragePaths() { + } + + /** + * Whether {@code path} lies inside one of our own storage roots, either of which may be null. + * + * @param path + * the candidate, need not exist + * @param external + * getExternalFilesDir(null), null while the volume is unmounted + * @param internal + * getFilesDir(), the fallback root + * @return true inside a root, false provably outside, and null when the question cannot be + * answered: with the external root unresolved there is nothing to compare against, and + * a caller that reads that as a refusal would discard a setting it cannot vet. + */ + static Boolean isWritable(final File path, final File external, final File internal) { + try { + // The separator on both sides is what keeps a sibling like "files2" from matching "files". + final String target = path.getCanonicalPath() + File.separator; + for (final File root : new File[] { external, internal }) { + if (root != null + && target.startsWith(root.getCanonicalPath() + File.separator)) { + return true; + } + } + } catch (final IOException e) { + return null; + } + return external != null ? Boolean.FALSE : null; + } +} diff --git a/app/src/main/res/xml/provider_paths.xml b/app/src/main/res/xml/provider_paths.xml index 0708c9a..30f751f 100644 --- a/app/src/main/res/xml/provider_paths.xml +++ b/app/src/main/res/xml/provider_paths.xml @@ -1,6 +1,10 @@ - + + - \ No newline at end of file + + + diff --git a/app/src/test/java/com/httrack/android/StoragePathsTest.java b/app/src/test/java/com/httrack/android/StoragePathsTest.java new file mode 100644 index 0000000..3bedbd7 --- /dev/null +++ b/app/src/test/java/com/httrack/android/StoragePathsTest.java @@ -0,0 +1,94 @@ +package com.httrack.android; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; + +import java.io.File; +import java.nio.file.Files; + +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +/** Attacks StoragePaths.isWritable: what it must accept, what it must refuse, what it cannot tell. */ +public class StoragePathsTest { + @Rule + public final TemporaryFolder tmp = new TemporaryFolder(); + + private File external; + private File internal; + + @Before + public void setUp() throws Exception { + external = tmp.newFolder("ext", "Android", "data", "com.httrack.android", "files"); + internal = tmp.newFolder("int", "data", "com.httrack.android", "files"); + } + + @Test + public void acceptsOurOwnRoots() { + assertEquals(Boolean.TRUE, StoragePaths.isWritable(new File(external, "Websites"), external, internal)); + assertEquals(Boolean.TRUE, StoragePaths.isWritable(new File(internal, "Websites"), external, internal)); + } + + @Test + public void acceptsTheRootItself() { + assertEquals(Boolean.TRUE, StoragePaths.isWritable(external, external, internal)); + } + + @Test + public void refusesTheOldPublicRoot() { + final File old = new File(tmp.getRoot(), "ext/HTTrack/Websites"); + assertEquals(Boolean.FALSE, StoragePaths.isWritable(old, external, internal)); + assertEquals(Boolean.FALSE, StoragePaths.isWritable(new File("/"), external, internal)); + } + + /** "files2" shares the string prefix of "files" without being inside it. */ + @Test + public void refusesASiblingSharingThePrefix() throws Exception { + final File sibling = tmp.newFolder("ext", "Android", "data", "com.httrack.android", "files2"); + assertEquals(Boolean.FALSE, StoragePaths.isWritable(new File(sibling, "Websites"), external, internal)); + } + + @Test + public void refusesANeighbourPackage() throws Exception { + final File other = tmp.newFolder("ext", "Android", "data", "com.httrack.android.evil", "files"); + assertEquals(Boolean.FALSE, StoragePaths.isWritable(other, external, internal)); + } + + @Test + public void refusesTraversalOutOfTheRoot() { + assertEquals(Boolean.FALSE, + StoragePaths.isWritable(new File(external, "../../../../HTTrack"), external, internal)); + } + + @Test + public void refusesASymlinkLeavingTheRoot() throws Exception { + final File outside = tmp.newFolder("outside"); + final File link = new File(external, "escape"); + Files.createSymbolicLink(link.toPath(), outside.toPath()); + assertEquals(Boolean.FALSE, StoragePaths.isWritable(link, external, internal)); + } + + /** + * The volume is gone, so an external path cannot be vetted. Answering "false" here would make + * the caller erase a good setting that a remount would have restored. + */ + @Test + public void cannotDecideWithoutTheExternalRoot() { + assertNull(StoragePaths.isWritable(new File(external, "Websites"), null, internal)); + } + + @Test + public void stillDecidesInternalPathsWithoutTheExternalRoot() { + assertEquals(Boolean.TRUE, StoragePaths.isWritable(new File(internal, "Websites"), null, internal)); + } + + /** Whatever the mount state, the default root the app picks must satisfy its own check. */ + @Test + public void theDefaultRootAlwaysPassesItsOwnCheck() { + assertTrue(StoragePaths.isWritable(new File(external, "Websites"), external, internal)); + assertTrue(StoragePaths.isWritable(new File(internal, "Websites"), null, internal)); + } +}