From 378c580a556605d50f645c0ae39f10bcaea56696 Mon Sep 17 00:00:00 2001 From: Xavier Roche Date: Wed, 21 Oct 2026 08:00:40 +0200 Subject: [PATCH 1/2] Delete the staged login copy when a project goes BrowserLoginActivity.writeOwnerOnly stages the cookies under .txt.new and renames it onto .txt, so a write that fails halfway cannot truncate a good login. If the app dies between the write and the rename, the staged file stays on disk. CleanupActivity.forgetLogin deletes every file StoragePaths.isLoginSibling accepts. Its pattern took the login name, an optional ".bak" and an optional "..tmp", which are the engine's own leftovers, and never the staged name. So deleting the project left a full session in private storage. The suffix lived as a literal in BrowserLoginActivity and the pattern lived in StoragePaths, which is how the two could disagree. Move the staged name into StoragePaths.stagedLoginFile and build the pattern from the same constant. Reported by the engine-side session while reviewing httrack #1992. Signed-off-by: Xavier Roche Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Xavier Roche --- .../httrack/android/BrowserLoginActivity.java | 6 ++--- .../com/httrack/android/StoragePaths.java | 25 ++++++++++++++++--- .../android/BrowserLoginWiringTest.java | 12 +++++++++ .../com/httrack/android/StoragePathsTest.java | 15 +++++++++-- 4 files changed, 49 insertions(+), 9 deletions(-) diff --git a/app/src/main/java/com/httrack/android/BrowserLoginActivity.java b/app/src/main/java/com/httrack/android/BrowserLoginActivity.java index c5d9d12..bfb26c4 100644 --- a/app/src/main/java/com/httrack/android/BrowserLoginActivity.java +++ b/app/src/main/java/com/httrack/android/BrowserLoginActivity.java @@ -265,8 +265,8 @@ public void onClickLoginDone(final View view) { /** * Write TEXT to FILE, readable by this app alone. * - * The file holds a live session, so it writes a .new beside it and renames, because a write - * that failed halfway would leave the previous login truncated and the crawl with nothing. + * The file holds a live session, so it stages beside FILE and renames, because a write that + * failed halfway would leave the previous login truncated and the crawl with nothing. * * @return true when FILE now holds TEXT */ @@ -275,7 +275,7 @@ static boolean writeOwnerOnly(final File file, final String text) { if (dir != null && !dir.isDirectory() && !dir.mkdirs()) { return false; } - final File staged = new File(file.getPath() + ".new"); + final File staged = StoragePaths.stagedLoginFile(file); try (final OutputStream out = new FileOutputStream(staged)) { out.write(text.getBytes(UTF8)); } catch (final IOException e) { diff --git a/app/src/main/java/com/httrack/android/StoragePaths.java b/app/src/main/java/com/httrack/android/StoragePaths.java index c38433f..30def74 100644 --- a/app/src/main/java/com/httrack/android/StoragePaths.java +++ b/app/src/main/java/com/httrack/android/StoragePaths.java @@ -276,14 +276,31 @@ static List savedLoginFiles(final File internal, final String projectName) return files; } + /** What {@link #stagedLoginFile} appends, which {@link #isLoginSibling} has to accept. */ + private static final String STAGED_SUFFIX = ".new"; + /** - * Whether FILENAME is LOGINNAME, or one of the files the engine's save leaves beside it. - * Those are its {@code .bak}, its {@code ..tmp}, and the temporary the {@code .bak} is - * staged through, because each one can hold a whole jar. No run deletes another's temporary. + * Whether FILENAME is LOGINNAME, or one of the files a save leaves beside it. The engine + * leaves its {@code .bak}, its {@code ..tmp}, and the temporary the {@code .bak} is + * staged through. Our own write leaves its staged copy when the app dies before the rename. + * Each one can hold a whole jar. No run deletes another's temporary. */ static boolean isLoginSibling(final String ourName, final String candidate) { return candidate != null - && candidate.matches(Pattern.quote(ourName) + "(\\.bak)?(\\.[0-9]+\\.tmp)?"); + && candidate.matches(Pattern.quote(ourName) + "(" + Pattern.quote(STAGED_SUFFIX) + + "|(\\.bak)?(\\.[0-9]+\\.tmp)?)"); + } + + /** + * Where a login write stages before it renames onto FILE. {@link #savedLoginFiles} takes this + * name too, because the app can die between the write and the rename. + * + * @param file + * the login file being written + * @return the name to write first + */ + static File stagedLoginFile(final File file) { + return new File(file.getPath() + STAGED_SUFFIX); } /** diff --git a/app/src/test/java/com/httrack/android/BrowserLoginWiringTest.java b/app/src/test/java/com/httrack/android/BrowserLoginWiringTest.java index 76726be..011dd67 100644 --- a/app/src/test/java/com/httrack/android/BrowserLoginWiringTest.java +++ b/app/src/test/java/com/httrack/android/BrowserLoginWiringTest.java @@ -143,6 +143,18 @@ public void deletingAProjectForgetsItsLogin() throws IOException { assertTrue(forget, forget.contains("StoragePaths.savedLoginFiles(")); } + /** A crash between the write and the rename leaves the staged copy for forgetLogin to take. */ + @Test + public void theStagedWriteUsesTheNameTheDeleteKnows() throws IOException { + final String source = TestSources + .withoutCommentsAndStrings(TestSources.javaSource("BrowserLoginActivity")); + final String body = TestSources.balancedBlock(source, + TestSources.indexOf(source, "static boolean writeOwnerOnly(")); + // The name comes from the helper isLoginSibling builds its pattern from, never a literal. + assertTrue(body, body.contains("StoragePaths.stagedLoginFile(file)")); + assertTrue(body, body.contains("staged.renameTo(file)")); + } + /** -%K is read only when the engine accepts cookies, so the hand-off has to tick the box. */ @Test public void theHandOverTurnsCookiesOn() throws IOException { diff --git a/app/src/test/java/com/httrack/android/StoragePathsTest.java b/app/src/test/java/com/httrack/android/StoragePathsTest.java index 70e6861..3a5a625 100644 --- a/app/src/test/java/com/httrack/android/StoragePathsTest.java +++ b/app/src/test/java/com/httrack/android/StoragePathsTest.java @@ -39,6 +39,14 @@ public void aDeletedLoginTakesWhatTheEngineLeftBesideIt() { assertTrue(StoragePaths.isLoginSibling("My Site.txt", "My Site.txt.bak.4321.tmp")); } + /** The matcher must take whatever our write stages, or a crash mid-save leaks the session. */ + @Test + public void aDeletedLoginTakesTheStagedWriteACrashLeftBehind() { + final File login = StoragePaths.loginCookiesFile(internal, "My Site"); + assertTrue(StoragePaths.isLoginSibling(login.getName(), + StoragePaths.stagedLoginFile(login).getName())); + } + @Test public void aDeletedLoginLeavesAnotherProjectAlone() { assertFalse(StoragePaths.isLoginSibling("A.txt", "B.txt")); @@ -49,6 +57,9 @@ public void aDeletedLoginLeavesAnotherProjectAlone() { assertFalse(StoragePaths.isLoginSibling("A.txt", "A.txt.bak2")); assertFalse(StoragePaths.isLoginSibling("A.txt", "A.txt.bak.tmp")); assertFalse(StoragePaths.isLoginSibling("A.txt", "A.txt.4321.bak")); + // The staged suffix stands alone, because nothing writes it onto a .bak or a temporary. + assertFalse(StoragePaths.isLoginSibling("A.txt", "A.txt.bak.new")); + assertFalse(StoragePaths.isLoginSibling("A.txt", "A.txt.new.new")); assertFalse(StoragePaths.isLoginSibling("A.txt", null)); /* isValidProjectName allows a regex metacharacter, so the name is quoted before it becomes a pattern. Unquoted, "A+" would match another project's "AA.txt" and delete its session, @@ -65,7 +76,7 @@ public void savedLoginFilesTakesOurOwnAndLeavesTheNeighbour() throws IOException final File logins = new File(internal, "logins"); assertTrue(logins.mkdirs()); for (final String name : new String[] { "P.txt", "P.txt.bak", "P.txt.4321.tmp", - "P.txt.bak.4321.tmp", "Q.txt", "Q.txt.bak", "notes.txt" }) { + "P.txt.bak.4321.tmp", "P.txt.new", "Q.txt", "Q.txt.bak", "Q.txt.new", "notes.txt" }) { assertTrue(new File(logins, name).createNewFile()); } final java.util.List ours = StoragePaths.savedLoginFiles(internal, "P"); @@ -74,7 +85,7 @@ public void savedLoginFilesTakesOurOwnAndLeavesTheNeighbour() throws IOException names.add(file.getName()); } assertEquals(new java.util.TreeSet(java.util.Arrays.asList("P.txt", "P.txt.bak", - "P.txt.4321.tmp", "P.txt.bak.4321.tmp")), names); + "P.txt.4321.tmp", "P.txt.bak.4321.tmp", "P.txt.new")), names); } @Test From a4712b014f6750cac5b800c64e441f39319de4ac Mon Sep 17 00:00:00 2001 From: Xavier Roche Date: Wed, 21 Oct 2026 08:11:21 +0200 Subject: [PATCH 2/2] Pin where the staged login lands, not just its name Review found two mutations the first round's tests let through. Staging into another directory under the same name kept the matcher agreeing with itself, so the assertion passed while savedLoginFiles, which lists only the logins directory, could no longer see the file. The leak was back with nothing red. Assert the whole path. Calling stagedLoginFile and then opening the stream on an open-coded name left both grepped substrings in place, so the wiring test stayed green while nothing was ever saved. Assert the stream is opened on the staged file. The test named a consequence it never read, so rename it after what it checks. isLoginSibling's first argument takes a login file name, not a project name, so say so in the name and state the summary in the parameters' own order. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Xavier Roche --- .../java/com/httrack/android/StoragePaths.java | 17 ++++++++--------- .../httrack/android/BrowserLoginWiringTest.java | 7 ++++--- .../com/httrack/android/StoragePathsTest.java | 2 ++ 3 files changed, 14 insertions(+), 12 deletions(-) diff --git a/app/src/main/java/com/httrack/android/StoragePaths.java b/app/src/main/java/com/httrack/android/StoragePaths.java index 30def74..9640e7a 100644 --- a/app/src/main/java/com/httrack/android/StoragePaths.java +++ b/app/src/main/java/com/httrack/android/StoragePaths.java @@ -276,24 +276,23 @@ static List savedLoginFiles(final File internal, final String projectName) return files; } - /** What {@link #stagedLoginFile} appends, which {@link #isLoginSibling} has to accept. */ + /** {@link #isLoginSibling} must accept the name {@link #stagedLoginFile} builds. */ private static final String STAGED_SUFFIX = ".new"; /** - * Whether FILENAME is LOGINNAME, or one of the files a save leaves beside it. The engine - * leaves its {@code .bak}, its {@code ..tmp}, and the temporary the {@code .bak} is - * staged through. Our own write leaves its staged copy when the app dies before the rename. - * Each one can hold a whole jar. No run deletes another's temporary. + * Whether LOGINFILENAME names CANDIDATE, or one of the files a save leaves beside it. The + * engine leaves its {@code .bak}, its {@code ..tmp}, and the temporary the {@code .bak} + * is staged through. Our own write leaves its staged copy when the app dies before the + * rename. Each one can hold a whole jar. No run deletes another's temporary. */ - static boolean isLoginSibling(final String ourName, final String candidate) { + static boolean isLoginSibling(final String loginFileName, final String candidate) { return candidate != null - && candidate.matches(Pattern.quote(ourName) + "(" + Pattern.quote(STAGED_SUFFIX) + && candidate.matches(Pattern.quote(loginFileName) + "(" + Pattern.quote(STAGED_SUFFIX) + "|(\\.bak)?(\\.[0-9]+\\.tmp)?)"); } /** - * Where a login write stages before it renames onto FILE. {@link #savedLoginFiles} takes this - * name too, because the app can die between the write and the rename. + * Where a login write stages before it renames onto FILE. * * @param file * the login file being written diff --git a/app/src/test/java/com/httrack/android/BrowserLoginWiringTest.java b/app/src/test/java/com/httrack/android/BrowserLoginWiringTest.java index 011dd67..4894866 100644 --- a/app/src/test/java/com/httrack/android/BrowserLoginWiringTest.java +++ b/app/src/test/java/com/httrack/android/BrowserLoginWiringTest.java @@ -143,15 +143,16 @@ public void deletingAProjectForgetsItsLogin() throws IOException { assertTrue(forget, forget.contains("StoragePaths.savedLoginFiles(")); } - /** A crash between the write and the rename leaves the staged copy for forgetLogin to take. */ + /** The staged name must come from the shared helper, so the delete pattern can match it. */ @Test - public void theStagedWriteUsesTheNameTheDeleteKnows() throws IOException { + public void theStagedWriteBuildsItsNameFromTheSharedHelper() throws IOException { final String source = TestSources .withoutCommentsAndStrings(TestSources.javaSource("BrowserLoginActivity")); final String body = TestSources.balancedBlock(source, TestSources.indexOf(source, "static boolean writeOwnerOnly(")); - // The name comes from the helper isLoginSibling builds its pattern from, never a literal. assertTrue(body, body.contains("StoragePaths.stagedLoginFile(file)")); + // On the staged file, because calling the helper and then writing elsewhere saves nothing. + assertTrue(body, body.contains("FileOutputStream(staged)")); assertTrue(body, body.contains("staged.renameTo(file)")); } diff --git a/app/src/test/java/com/httrack/android/StoragePathsTest.java b/app/src/test/java/com/httrack/android/StoragePathsTest.java index 3a5a625..8660226 100644 --- a/app/src/test/java/com/httrack/android/StoragePathsTest.java +++ b/app/src/test/java/com/httrack/android/StoragePathsTest.java @@ -43,6 +43,8 @@ public void aDeletedLoginTakesWhatTheEngineLeftBesideIt() { @Test public void aDeletedLoginTakesTheStagedWriteACrashLeftBehind() { final File login = StoragePaths.loginCookiesFile(internal, "My Site"); + // Beside the login, because savedLoginFiles lists that one directory and nothing else. + assertEquals(new File(login.getPath() + ".new"), StoragePaths.stagedLoginFile(login)); assertTrue(StoragePaths.isLoginSibling(login.getName(), StoragePaths.stagedLoginFile(login).getName())); }