aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorDanilo M. <danix@danix.xyz>2026-11-03 14:44:58 +0100
committerDanilo M. <danix@danix.xyz>2026-11-03 14:44:58 +0100
commit2b73e524d9624897f675ccc14f614fc450a7565f (patch)
tree5f90dd4963e0aaeb2c699d86e1a2acc9dceaf366
parent2911783653f699e41a07bb9536460d9d0a1c3b70 (diff)
downloadHardenedPass-2b73e524d9624897f675ccc14f614fc450a7565f.tar.gz
HardenedPass-2b73e524d9624897f675ccc14f614fc450a7565f.zip
fix(app): address the step 5 review and emulator findings
- A store a cancelled attempt already made for the chosen key (its .gpg-id names exactly that key) no longer counts as occupied: choosing the key again commits and moves on instead of refusing. - SSH key import reads at most 100 kB whatever the provider reports, like the PGP import, instead of reading the stream whole. - PGPKeyManager replaces a key through a temporary file moved over it atomically, so a failed write keeps the old key; listings only read .key files, so a stray temporary file is never taken for a key. - "View generated public SSH key" goes through the settings result launcher, so the preference visibility refreshes afterwards. - A plain start from LaunchActivity replaces the task: an open MainActivity (e.g. one whose store vanished) is no longer left below the new one, and one back leaves the app. - The PGP keys screen's import FAB carries its content description itself; M3 cleared the icon's. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
-rw-r--r--app/src/main/java/app/passwordstore/ui/main/LaunchActivity.kt3
-rw-r--r--app/src/main/java/app/passwordstore/ui/settings/RepositorySettings.kt2
-rw-r--r--app/src/main/java/app/passwordstore/ui/setup/PgpKeysScreen.kt14
-rw-r--r--app/src/main/java/app/passwordstore/ui/setup/PgpKeysViewModel.kt10
-rw-r--r--app/src/main/java/app/passwordstore/ui/setup/SetupModel.kt7
-rw-r--r--app/src/main/java/app/passwordstore/util/git/sshj/SshKey.kt14
-rw-r--r--app/src/test/java/app/passwordstore/ui/setup/SetupModelTest.kt10
-rw-r--r--crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/PGPKeyManager.kt32
-rw-r--r--crypto/pgpainless/src/test/kotlin/app/passwordstore/crypto/PGPKeyManagerTest.kt5
9 files changed, 81 insertions, 16 deletions
diff --git a/app/src/main/java/app/passwordstore/ui/main/LaunchActivity.kt b/app/src/main/java/app/passwordstore/ui/main/LaunchActivity.kt
index befd37f9..66b5cf1f 100644
--- a/app/src/main/java/app/passwordstore/ui/main/LaunchActivity.kt
+++ b/app/src/main/java/app/passwordstore/ui/main/LaunchActivity.kt
@@ -56,6 +56,9 @@ class LaunchActivity : AppCompatActivity() {
val intentToStart = Intent(this, MainActivity::class.java)
if (intent.action == ACTION_DECRYPT_PASS && isFileInRepo(filePath, repoDir))
intentToStart.putExtra(BasePGPActivity.EXTRA_FILE_PATH, filePath)
+ // A plain start replaces the task, so an instance already open (one whose store vanished, say)
+ // is not left below the new one. An entry opens on top, back returns to where the user was.
+ else intentToStart.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TASK)
startActivity(intentToStart)
Handler(Looper.getMainLooper()).postDelayed({ finish() }, if (noAuth) 0L else 500L)
diff --git a/app/src/main/java/app/passwordstore/ui/settings/RepositorySettings.kt b/app/src/main/java/app/passwordstore/ui/settings/RepositorySettings.kt
index af50b3ea..23dd2829 100644
--- a/app/src/main/java/app/passwordstore/ui/settings/RepositorySettings.kt
+++ b/app/src/main/java/app/passwordstore/ui/settings/RepositorySettings.kt
@@ -110,7 +110,7 @@ class RepositorySettings(private val activity: FragmentActivity) : SettingsProvi
titleRes = R.string.pref_ssh_see_key_title
visible = PasswordRepository.isGitRepo() && SshKey.canShowSshPublicKey
onClick {
- activity.startActivity(MainActivity.setupIntent(activity, SetupStart.Ssh))
+ generateSshKey.launch(MainActivity.setupIntent(activity, SetupStart.Ssh))
true
}
}
diff --git a/app/src/main/java/app/passwordstore/ui/setup/PgpKeysScreen.kt b/app/src/main/java/app/passwordstore/ui/setup/PgpKeysScreen.kt
index 34e5bed6..11fd979c 100644
--- a/app/src/main/java/app/passwordstore/ui/setup/PgpKeysScreen.kt
+++ b/app/src/main/java/app/passwordstore/ui/setup/PgpKeysScreen.kt
@@ -43,6 +43,8 @@ import androidx.compose.runtime.setValue
import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
import androidx.compose.ui.res.stringResource
+import androidx.compose.ui.semantics.contentDescription
+import androidx.compose.ui.semantics.semantics
import androidx.compose.ui.text.font.FontFamily
import androidx.compose.ui.text.style.TextOverflow
import androidx.compose.ui.unit.dp
@@ -121,11 +123,13 @@ fun PgpKeysScreen(
)
},
floatingActionButton = {
- // M3 clears the label's semantics; the FAB takes its description from the icon.
+ // M3 clears the semantics of the FAB's content, label and icon alike: describe the FAB.
+ val label = stringResource(R.string.pref_import_pgp_key_title)
ExtendedFloatingActionButton(
- text = { Text(stringResource(R.string.pref_import_pgp_key_title)) },
- icon = { Icon(Icons.Filled.Add, stringResource(R.string.pref_import_pgp_key_title)) },
+ text = { Text(label) },
+ icon = { Icon(Icons.Filled.Add, null) },
onClick = { if (!busy) onImport() },
+ modifier = Modifier.semantics { contentDescription = label },
)
},
) { padding ->
@@ -254,8 +258,8 @@ fun PgpKeysDestination(
if (uri != null) viewModel.import(uri, activity)
}
- // ponytail: a theme or locale change in the middle cancels the commit; the next commit picks up
- // the new `.gpg-id`, and choosing the key again re-creates nothing that would conflict.
+ // ponytail: a theme or locale change in the middle cancels the commit. Choosing the same key
+ // again finds the store already made for it (createdFor), commits the `.gpg-id` and moves on.
fun choose(row: KeyRow) {
if (viewModel.busy) return
viewModel.busy = true
diff --git a/app/src/main/java/app/passwordstore/ui/setup/PgpKeysViewModel.kt b/app/src/main/java/app/passwordstore/ui/setup/PgpKeysViewModel.kt
index d270275f..8f1b52ec 100644
--- a/app/src/main/java/app/passwordstore/ui/setup/PgpKeysViewModel.kt
+++ b/app/src/main/java/app/passwordstore/ui/setup/PgpKeysViewModel.kt
@@ -209,12 +209,16 @@ constructor(
/**
* Creates the local store at [root] for [row]: a git repository and a `.gpg-id` naming the key by
* its uppercase long key ID ([gpgIdText]). Returns null, or why it failed; [occupied] is the
- * reason given when [root] already holds a store.
+ * reason given when [root] already holds a store, unless that store was made for [row] already.
*/
suspend fun createStore(root: File, row: KeyRow, occupied: String): String? =
withContext(dispatcherProvider.io()) {
- // A restored, already-cloned store must keep its .gpg-id.
- if (root.isDirectory && cloneTarget(root) == CloneTarget.Occupied) return@withContext occupied
+ if (root.isDirectory && cloneTarget(root) == CloneTarget.Occupied) {
+ // Made by an earlier attempt for this key whose commit a recreation cancelled: done.
+ if (createdFor(root, row.keyId)) return@withContext null
+ // A restored, already-cloned store must keep its .gpg-id.
+ return@withContext occupied
+ }
try {
check(root.isDirectory || root.mkdirs()) { "Could not create $root" }
PasswordRepository.createRepository(root)
diff --git a/app/src/main/java/app/passwordstore/ui/setup/SetupModel.kt b/app/src/main/java/app/passwordstore/ui/setup/SetupModel.kt
index ade23aa2..4e32505f 100644
--- a/app/src/main/java/app/passwordstore/ui/setup/SetupModel.kt
+++ b/app/src/main/java/app/passwordstore/ui/setup/SetupModel.kt
@@ -95,6 +95,13 @@ fun storeRecipients(text: String): List<PGPIdentifier> =
fun gpgIdText(keys: List<PGPIdentifier.KeyId>): String =
keys.joinToString("") { "${it.toString().uppercase(Locale.ROOT)}\n" }
+/**
+ * Whether [dir] holds a store already made for [key]: its `.gpg-id` is exactly what creating one
+ * for [key] writes. Blocking: call it off the main thread.
+ */
+fun createdFor(dir: File, key: PGPIdentifier.KeyId): Boolean =
+ readGpgId(dir) == gpgIdText(listOf(key))
+
private const val MAX_GPG_ID_BYTES = 64 * 1024
/**
diff --git a/app/src/main/java/app/passwordstore/util/git/sshj/SshKey.kt b/app/src/main/java/app/passwordstore/util/git/sshj/SshKey.kt
index 06cf5bc8..7c8f5ba5 100644
--- a/app/src/main/java/app/passwordstore/util/git/sshj/SshKey.kt
+++ b/app/src/main/java/app/passwordstore/util/git/sshj/SshKey.kt
@@ -16,6 +16,7 @@ import android.util.Base64
import androidx.core.content.edit
import app.passwordstore.Application
import app.passwordstore.R
+import app.passwordstore.ui.setup.readAtMost
import app.passwordstore.util.crypto.keystoreAesKey
import app.passwordstore.util.crypto.requireUserAuthentication
import app.passwordstore.util.crypto.seal
@@ -239,7 +240,18 @@ object SshKey {
val sshKeyInputStream =
context.contentResolver.openInputStream(uri)
?: throw IOException(context.getString(R.string.ssh_key_does_not_exist))
- val lines = sshKeyInputStream.use { `is` -> `is`.bufferedReader().readLines() }
+ // Bounded whatever the provider claimed above: a larger stream is refused, not read whole.
+ val bytes =
+ sshKeyInputStream.use { readAtMost(it, 100_000) }
+ ?: throw IllegalArgumentException(
+ context.getString(R.string.ssh_key_import_error_not_an_ssh_key_message)
+ )
+ val lines =
+ try {
+ bytes.inputStream().bufferedReader().readLines()
+ } finally {
+ bytes.fill(0)
+ }
// Private key markers and a format sshj can read; the current key stays when this fails.
if (!isSshPrivateKey(lines))
diff --git a/app/src/test/java/app/passwordstore/ui/setup/SetupModelTest.kt b/app/src/test/java/app/passwordstore/ui/setup/SetupModelTest.kt
index 53ea80cf..9ccddbd3 100644
--- a/app/src/test/java/app/passwordstore/ui/setup/SetupModelTest.kt
+++ b/app/src/test/java/app/passwordstore/ui/setup/SetupModelTest.kt
@@ -137,4 +137,14 @@ class SetupModelTest {
val folder = File(root, "folder").apply { File(this, ".gpg-id").mkdirs() }
assertNull(readGpgId(folder))
}
+
+ @Test
+ fun createdForMatchesOnlyTheChosenKeysGpgId() {
+ val key = PGPIdentifier.fromString("0xf8993a789cdfb014") as PGPIdentifier.KeyId
+ assertEquals(false, createdFor(root, key))
+ File(root, ".gpg-id").writeText(gpgIdText(listOf(key)))
+ assertEquals(true, createdFor(root, key))
+ File(root, ".gpg-id").writeText("F8993A789CDFB014\nB950AE2813841585\n")
+ assertEquals(false, createdFor(root, key))
+ }
}
diff --git a/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/PGPKeyManager.kt b/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/PGPKeyManager.kt
index ef58ad19..33d61343 100644
--- a/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/PGPKeyManager.kt
+++ b/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/PGPKeyManager.kt
@@ -22,6 +22,8 @@ import com.github.michaelbull.result.Result
import com.github.michaelbull.result.coroutines.runSuspendCatching
import com.github.michaelbull.result.unwrap
import java.io.File
+import java.nio.file.Files
+import java.nio.file.StandardCopyOption
import javax.inject.Inject
import kotlinx.coroutines.CoroutineDispatcher
import kotlinx.coroutines.withContext
@@ -51,13 +53,13 @@ constructor(filesDir: String, private val dispatcher: CoroutineDispatcher) :
tryParseKeyring(PGPKey(existingKeyBytes)) ?: throw InvalidKeyException
when {
existingKeyRing is PGPPublicKeyRing && incomingKeyRing is PGPSecretKeyRing -> {
- keyFile.writeBytes(key.contents)
+ writeAtomically(keyFile, key.contents)
return@runSuspendCatching key
}
existingKeyRing is PGPPublicKeyRing && incomingKeyRing is PGPPublicKeyRing -> {
val updatedPublicKey = PGPainless.mergeCertificate(existingKeyRing, incomingKeyRing)
val keyBytes = PGPainless.asciiArmor(updatedPublicKey).encodeToByteArray()
- keyFile.writeBytes(keyBytes)
+ writeAtomically(keyFile, keyBytes)
return@runSuspendCatching key
}
// Never with replace either: it would delete the only copy of the private key.
@@ -69,10 +71,9 @@ constructor(filesDir: String, private val dispatcher: CoroutineDispatcher) :
throw KeyAlreadyExistsException(
tryGetId(key)?.toString() ?: "Failed to retrieve key ID"
)
- if (!keyFile.delete()) throw KeyDeletionFailedException
}
- keyFile.writeBytes(key.contents)
+ writeAtomically(keyFile, key.contents)
key
}
@@ -96,7 +97,7 @@ constructor(filesDir: String, private val dispatcher: CoroutineDispatcher) :
withContext(dispatcher) {
runSuspendCatching {
if (!keyDirExists()) throw KeyDirectoryUnavailableException
- val keyFiles = keyDir.listFiles()
+ val keyFiles = keyDir.listFiles { file -> file.extension == KEY_EXTENSION }
if (keyFiles.isNullOrEmpty()) throw NoKeysAvailableException
val keys = keyFiles.map { file -> PGPKey(file.readBytes()) }
@@ -134,7 +135,7 @@ constructor(filesDir: String, private val dispatcher: CoroutineDispatcher) :
withContext(dispatcher) {
runSuspendCatching {
if (!keyDirExists()) throw KeyDirectoryUnavailableException
- val keyFiles = keyDir.listFiles()
+ val keyFiles = keyDir.listFiles { file -> file.extension == KEY_EXTENSION }
if (keyFiles.isNullOrEmpty()) return@runSuspendCatching emptyList()
keyFiles.map { keyFile -> PGPKey(keyFile.readBytes()) }.toList()
}
@@ -143,6 +144,25 @@ constructor(filesDir: String, private val dispatcher: CoroutineDispatcher) :
/** @see KeyManager.getKeyById */
override suspend fun getKeyId(key: PGPKey): PGPIdentifier? = tryGetId(key)
+ /**
+ * Writes [bytes] to a temporary file next to [file] and moves it over [file] in one step, so a
+ * failed write keeps the key that was there.
+ */
+ private fun writeAtomically(file: File, bytes: ByteArray) {
+ val temp = File(file.parentFile, ".${file.name}.tmp")
+ try {
+ temp.writeBytes(bytes)
+ Files.move(
+ temp.toPath(),
+ file.toPath(),
+ StandardCopyOption.REPLACE_EXISTING,
+ StandardCopyOption.ATOMIC_MOVE,
+ )
+ } finally {
+ temp.delete()
+ }
+ }
+
/** Checks if [keyDir] exists and attempts to create it if not. */
private fun keyDirExists(): Boolean {
return keyDir.exists() || keyDir.mkdirs()
diff --git a/crypto/pgpainless/src/test/kotlin/app/passwordstore/crypto/PGPKeyManagerTest.kt b/crypto/pgpainless/src/test/kotlin/app/passwordstore/crypto/PGPKeyManagerTest.kt
index 81a666f4..587f7239 100644
--- a/crypto/pgpainless/src/test/kotlin/app/passwordstore/crypto/PGPKeyManagerTest.kt
+++ b/crypto/pgpainless/src/test/kotlin/app/passwordstore/crypto/PGPKeyManagerTest.kt
@@ -72,6 +72,11 @@ class PGPKeyManagerTest {
val keyId = keyManager.getKeyId(keyManager.addKey(secretKey, true).unwrap())
assertEquals(KeyId(CryptoConstants.KEY_ID), keyId)
+ // Replaced through a temporary file that does not stay behind.
+ assertEquals(listOf("$keyId.${PGPKeyManager.KEY_EXTENSION}"), keysDir.list()?.toList())
+ // A stray temporary file left by a crash is not listed as a key.
+ File(keysDir, ".$keyId.${PGPKeyManager.KEY_EXTENSION}.tmp").writeText("partial")
+ assertEquals(1, keyManager.getAllKeys().unwrap().size)
}
@Test