Write mirrors to the app-specific external directory (scoped storage)

.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

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'
 }

app/src/main/AndroidManifest.xml

@@ -2,9 +2,6 @@
 <manifest xmlns:android="http://schemas.android.com/apk/res/android"
     android:installLocation="auto" >
 
-    <!-- We need to read/write copied Websites to the SD card. -->
-    <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" />
-
     <!-- We need to connect to the Internet. -->
     <uses-permission android:name="android.permission.INTERNET" />
 

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<Pair<String, File>> files = new ArrayList<Pair<String, File>>();
 
@@ -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");
     }

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 <http://stackoverflow.com/questions/33139754/android-6-0-marshmallow-cannot-write-to-sd-card>
-  * 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);
   }
 

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;
+  }
+}

app/src/main/res/xml/provider_paths.xml

@@ -1,6 +1,10 @@
 <?xml version="1.0" encoding="utf-8"?>
 <paths xmlns:android="http://schemas.android.com/apk/res/android">
-    <external-path name="external_path" path="."/>
+    <!-- Mirrors: getExternalFilesDir(). -->
     <external-files-path name="external_files_path" path="." />
+    <!-- Mirrors again, on the internal fallback taken while the volume is unmounted. -->
     <files-path name="files_path" path="." />
-</paths>
+    <!-- Unpacked help and license pages: getExternalCacheDir(). Browsing either throws
+         "Failed to find configured root" without this, once the file:// hack stops working. -->
+    <external-cache-path name="external_cache_path" path="." />
+</paths>

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));
+  }
+}