From 0c259be4d69c8b5df8140e311a99facc76ff4ae1 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 1 Sep 2026 10:40:46 +0000 Subject: [PATCH 1/8] Bump russh from 0.62.6 to 0.63.1 in /apps/linux in the linux-app group Bumps the linux-app group in /apps/linux with 1 update: [russh](https://github.com/warp-tech/russh). Updates `russh` from 0.62.6 to 0.63.1 - [Release notes](https://github.com/warp-tech/russh/releases) - [Commits](https://github.com/warp-tech/russh/compare/v0.62.6...v0.63.1) --- updated-dependencies: - dependency-name: russh dependency-version: 0.63.1 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: linux-app ... Signed-off-by: dependabot[bot] --- Cargo.toml | 2 +- apps/linux/Cargo.lock | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 4028e7c..489a6a9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -26,7 +26,7 @@ sg-sync = { path = "crates/sg-sync" } anyhow = "1.0" async-trait = "0.1" -russh = "0.62" +russh = "0.63" # Pairing crypto. X25519 for the key agreement, ChaCha20-Poly1305 for the payload — both already # pulled in transitively by russh, so this adds no new supply chain. x25519-dalek = { version = "3.0", features = ["static_secrets"] } diff --git a/apps/linux/Cargo.lock b/apps/linux/Cargo.lock index 0eeedfa..934e1f7 100644 --- a/apps/linux/Cargo.lock +++ b/apps/linux/Cargo.lock @@ -1936,9 +1936,9 @@ dependencies = [ [[package]] name = "russh" -version = "0.62.6" +version = "0.63.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b41043523e0edcbd4e31d00903e26f12994f63b21bae9904f7405c1ed92752a5" +checksum = "35bab1b87d915817d5d9cc352637cd40d5f0b298a48c6309af9156a4addc3031" dependencies = [ "aes", "aws-lc-rs", From 25b1e91b66b972969bc24983045dd4948048ff4b Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 1 Sep 2026 10:40:46 +0000 Subject: [PATCH 2/8] Bump russh from 0.62.6 to 0.63.1 in the rust group Bumps the rust group with 1 update: [russh](https://github.com/warp-tech/russh). Updates `russh` from 0.62.6 to 0.63.1 - [Release notes](https://github.com/warp-tech/russh/releases) - [Commits](https://github.com/warp-tech/russh/compare/v0.62.6...v0.63.1) --- updated-dependencies: - dependency-name: russh dependency-version: 0.63.1 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: rust ... Signed-off-by: dependabot[bot] --- Cargo.lock | 4 ++-- Cargo.toml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3aa1c54..652f0d7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1539,9 +1539,9 @@ dependencies = [ [[package]] name = "russh" -version = "0.62.6" +version = "0.63.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b41043523e0edcbd4e31d00903e26f12994f63b21bae9904f7405c1ed92752a5" +checksum = "35bab1b87d915817d5d9cc352637cd40d5f0b298a48c6309af9156a4addc3031" dependencies = [ "aes", "aws-lc-rs", diff --git a/Cargo.toml b/Cargo.toml index 4028e7c..489a6a9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -26,7 +26,7 @@ sg-sync = { path = "crates/sg-sync" } anyhow = "1.0" async-trait = "0.1" -russh = "0.62" +russh = "0.63" # Pairing crypto. X25519 for the key agreement, ChaCha20-Poly1305 for the payload — both already # pulled in transitively by russh, so this adds no new supply chain. x25519-dalek = { version = "3.0", features = ["static_secrets"] } From 9ea30493b7170be9f54bbb5157f8c903781f9894 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 1 Sep 2026 10:40:58 +0000 Subject: [PATCH 3/8] Bump actions/setup-java from 5 to 6 in the actions group Bumps the actions group with 1 update: [actions/setup-java](https://github.com/actions/setup-java). Updates `actions/setup-java` from 5 to 6 - [Release notes](https://github.com/actions/setup-java/releases) - [Commits](https://github.com/actions/setup-java/compare/v5...v6) --- updated-dependencies: - dependency-name: actions/setup-java dependency-version: '6' dependency-type: direct:production update-type: version-update:semver-major dependency-group: actions ... Signed-off-by: dependabot[bot] --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ea0b9e2..a83199a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -120,7 +120,7 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v7 - - uses: actions/setup-java@v5 + - uses: actions/setup-java@v6 with: distribution: temurin java-version: "21" From 422431fbbac3ccdff42b6bca1c762fb1c9110dbf Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 1 Sep 2026 10:41:16 +0000 Subject: [PATCH 4/8] Bump the android group in /apps/android with 2 updates Bumps the android group in /apps/android with 2 updates: com.android.application and [org.json:json](https://github.com/douglascrockford/JSON-java). Updates `com.android.application` from 9.3.1 to 9.3.2 Updates `org.json:json` from 20260719 to 20260814 - [Release notes](https://github.com/douglascrockford/JSON-java/releases) - [Changelog](https://github.com/stleary/JSON-java/blob/master/docs/RELEASES.md) - [Commits](https://github.com/douglascrockford/JSON-java/compare/20260719...20260814) --- updated-dependencies: - dependency-name: com.android.application dependency-version: 9.3.2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: android - dependency-name: org.json:json dependency-version: '20260814' dependency-type: direct:production update-type: version-update:semver-major dependency-group: android ... Signed-off-by: dependabot[bot] --- apps/android/app/build.gradle.kts | 2 +- apps/android/build.gradle.kts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/android/app/build.gradle.kts b/apps/android/app/build.gradle.kts index 0f7853e..95f1a5c 100644 --- a/apps/android/app/build.gradle.kts +++ b/apps/android/app/build.gradle.kts @@ -86,7 +86,7 @@ dependencies { // The record format is pure Kotlin plus org.json, so it is tested on the JVM rather than on a // device: a test that needs an emulator is a test that does not run. testImplementation("junit:junit:4.13.2") - testImplementation("org.json:json:20260719") + testImplementation("org.json:json:20260814") // Passwords and passphrases, encrypted with a key held in the Android Keystore. implementation("androidx.security:security-crypto:1.1.0") diff --git a/apps/android/build.gradle.kts b/apps/android/build.gradle.kts index 252cd64..c4be79c 100644 --- a/apps/android/build.gradle.kts +++ b/apps/android/build.gradle.kts @@ -1,5 +1,5 @@ plugins { - id("com.android.application") version "9.3.1" apply false + id("com.android.application") version "9.3.2" apply false id("org.jetbrains.kotlin.android") version "2.4.10" apply false id("org.jetbrains.kotlin.plugin.compose") version "2.4.10" apply false } From ef57b51d9cacc7040d3ee1a0cb77124a4e023ffc Mon Sep 17 00:00:00 2001 From: lazarevtill Date: Mon, 28 Sep 2026 12:13:09 +0200 Subject: [PATCH 5/8] Fix native packaging and cross-platform reliability --- .github/workflows/ci.yml | 44 +++-- CLAUDE.md | 14 +- README.md | 5 +- apps/Package.swift | 6 +- apps/android/app/build.gradle.kts | 10 +- .../cloud/lazarev/serverglass/AddServer.kt | 15 +- .../cloud/lazarev/serverglass/HostStore.kt | 101 ++++++----- .../cloud/lazarev/serverglass/MainActivity.kt | 19 +- .../kotlin/cloud/lazarev/serverglass/Model.kt | 165 ++++++++++------- .../kotlin/cloud/lazarev/serverglass/Ui.kt | 10 +- .../lazarev/serverglass/HostStoreTest.kt | 12 ++ apps/android/build.gradle.kts | 1 - apps/linux/Cargo.lock | 145 ++++----------- apps/linux/Cargo.toml | 3 +- apps/linux/src/palette.rs | 2 +- apps/linux/src/store.rs | 5 + apps/linux/src/views.rs | 7 +- apps/linux/src/window.rs | 2 +- apps/shared/ServerGlassUI/AddHostSheet.swift | 22 ++- apps/shared/ServerGlassUI/ContentView.swift | 60 +++++-- apps/shared/ServerGlassUI/CoreModel.swift | 49 +++-- apps/shared/ServerGlassUI/DesignSystem.swift | 9 +- apps/shared/ServerGlassUI/HostStore.swift | 29 +-- .../shared/ServerGlassUI/SimpleHostView.swift | 10 +- .../ServerGlassUITests/CoreBridgeTests.swift | 23 +++ .../ServerGlassUITests/HostStoreTests.swift | 28 ++- .../ServerGlass.Core.Tests/CoreTests.cs | 23 +++ .../ServerGlass.Core.Tests/HostStoreTests.cs | 2 + apps/windows/ServerGlass.Core/Core.cs | 126 ++++++++----- apps/windows/ServerGlass.Core/HostStore.cs | 13 +- apps/windows/ServerGlass/CoreModel.cs | 43 +++-- apps/windows/ServerGlass/Theme.cs | 6 +- apps/windows/ServerGlass/Views/PlainView.cs | 4 +- crates/sg-core/src/rate.rs | 33 +++- crates/sg-core/src/runtime.rs | 9 +- crates/sg-core/src/store.rs | 47 ++++- crates/sg-ffi/src/lib.rs | 170 +++++++++++++----- crates/sg-ffi/src/view.rs | 13 ++ crates/sg-sync/src/pairing.rs | 9 +- crates/sg-sync/src/transfer.rs | 153 +++++++++++----- crates/sg-sync/tests/pairing_over_tcp.rs | 65 +++++++ crates/sg-transport/src/error.rs | 8 + crates/sg-transport/src/frame.rs | 2 +- crates/sg-transport/src/session.rs | 162 ++++++++++------- crates/sg-transport/tests/live.rs | 79 +++++++- docs/RELIABILITY_AUDIT.md | 71 ++++++++ scripts/build-android.sh | 2 +- scripts/build-ios.sh | 8 +- scripts/build-macos.sh | 21 +++ scripts/release.sh | 3 +- 50 files changed, 1297 insertions(+), 571 deletions(-) create mode 100644 apps/shared/ServerGlassUITests/CoreBridgeTests.swift create mode 100644 docs/RELIABILITY_AUDIT.md diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a83199a..3a70712 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,15 +1,5 @@ -# The same checks the canonical GitLab pipeline runs, for the public GitHub mirror. -# -# Kept deliberately in step with .gitlab-ci.yml: the core on Linux and on Windows, the Linux app in -# a job of its own because it is a separate cargo workspace that needs GTK, and the Windows app in -# one of its own because it is C# and needs a .NET SDK. If you change one, change the other — a -# mirror whose CI checks less than the original is worse than no mirror, because the green tick -# claims something it did not verify. -# -# One job runs here and not there, deliberately: `android`. GitHub's Linux runners ship the Android -# SDK and NDK preinstalled and GitLab's docker executor does not, so this is the only place the -# F-Droid build path can be exercised without standing up a new image. That path is otherwise -# untested by anything automatic, and it is the one a rejected submission would be traced back to. +# Core and native front-end checks. Apple packaging and the Linux SSH fixtures are required +# here because compiling alone cannot detect a shipped dylib path or a skipped live test. name: CI on: @@ -44,10 +34,28 @@ jobs: - run: cargo fmt --all -- --check - run: cargo clippy --workspace --all-targets --locked -- -D warnings - run: cargo build --workspace --locked - # SG_REQUIRE_FIXTURES is deliberately not set: the live SSH tests need the Docker fixtures, - # detect their absence and skip. Everything that does not need a real host still runs. + - name: Start the live SSH fixtures + if: runner.os == 'Linux' + run: | + ./fixtures/up.sh + echo "SG_REQUIRE_FIXTURES=1" >> "$GITHUB_ENV" + # Windows runs the portable tests; Linux must also prove the one-round-trip SSH path. - run: cargo test --workspace --locked + macos-app: + name: macOS app and native bridge + runs-on: macos-14 + steps: + - uses: actions/checkout@v7 + - name: Install Rust + run: | + rustup toolchain install ${{ env.RUST_VERSION }} --profile minimal + rustup default ${{ env.RUST_VERSION }} + - uses: Swatinem/rust-cache@v2 + # Packaging asserts that the executable embeds the core, with no checkout-specific dylib. + - run: ./scripts/build-macos.sh + - run: swift test --package-path apps + linux-app: name: Linux app # 24.04 ships GTK 4.14 and libadwaita 1.5; the app needs 4.10 and 1.4 at least. @@ -65,8 +73,12 @@ jobs: - uses: Swatinem/rust-cache@v2 with: workspaces: apps/linux + - name: Start the live SSH fixtures + run: ./fixtures/up.sh - name: Check the app working-directory: apps/linux + env: + SG_REQUIRE_FIXTURES: "1" run: | cargo fmt -- --check cargo clippy --all-targets --locked -- -D warnings @@ -127,8 +139,8 @@ jobs: # Pinned, and installed by the action rather than taken from the runner image. There is no # gradle-wrapper.jar in this repository on purpose — it is a committed binary, which is the # one thing an F-Droid submission must not contain — so without this the build would run on - # whatever Gradle the image happened to ship that week. 9.7.0 is what the local build and the - # clean-clone dry run both used against AGP 8.9.0. + # whatever Gradle the image happened to ship that week. 9.7.0 is the version used by the + # local build against AGP 9.3.2. - uses: gradle/actions/setup-gradle@v6 with: gradle-version: "9.7.0" diff --git a/CLAUDE.md b/CLAUDE.md index f13b391..a9e870a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -61,7 +61,7 @@ hard way: ## Verifying ```bash -./scripts/check.sh # fmt, clippy, build, 272 tests +./scripts/check.sh # fmt, clippy, build, tests ./scripts/check.sh --all # plus Swift and Kotlin .\scripts\check.ps1 -All # the same, on Windows, plus the WinUI app ./scripts/build-linux.sh --run # build and run the GTK app @@ -101,12 +101,12 @@ The live tests need the fixtures: `./fixtures/up.sh` (Debian and Alpine sshd con GNU-versus-busybox parsing differences). Without `SG_REQUIRE_FIXTURES=1` they skip themselves and report `ok`, which has already hidden a broken container once. -**CI is GitLab (`.gitlab-ci.yml`, gitlab.lazarev.cloud) and runs the Rust jobs on Linux and -Windows, the Linux app on Linux, and the Windows app on Windows.** No macOS or Android runner is -configured, so the Swift and Kotlin suites are yours to run before pushing. The Windows jobs are a -shell runner that installs its own toolchain — `rust:windows` is the only thing checking that the -core compiles for a platform none of us develops on, and it earned that on its first run by -catching a Unix-only agent call. +**CI is defined in [`.github/workflows/ci.yml`](.github/workflows/ci.yml).** It runs the Rust +suite on Linux and Windows, builds and tests the Linux, Windows, macOS and Android front-ends, +and requires live SSH fixtures on Linux. The macOS build checks that the packaged executable +embeds the Rust core instead of loading a library from a developer checkout. The Swift tests +exercise that bridge. iOS still needs a local simulator build and launch check. Windows UI +behavior and DPAPI must be checked on Windows; cross-platform core tests cannot verify them. Check the pipeline after pushing. It was left red for eight commits once, because tests passing locally says nothing about `fmt` and `clippy`. diff --git a/README.md b/README.md index d69d4af..58b5a49 100644 --- a/README.md +++ b/README.md @@ -326,9 +326,8 @@ was, which is exactly what a parser bug is made of. Both a GNU and a BusyBox hos - Setup still assumes someone who knows what a hostname and an SSH key are. The *reading* experience is written for a non-technical person; the *adding* experience is not yet. -- CI runs the Rust suite on Linux and on Windows, and builds and tests the Linux and Windows apps. - The Swift and Kotlin tests exist and pass locally, but no macOS or Android runner is configured - to run them. +- CI builds and tests the core and the Linux, Windows, macOS and Android front-ends. iOS still + needs local simulator verification, and native UI behavior needs a launch check on each platform. - The Windows app cannot scan a pairing QR, because a desktop has no camera. It shows one to receive an inventory and takes a pasted code to send one. - The Linux app keeps a password or key passphrase in memory for the run rather than in the Secret diff --git a/apps/Package.swift b/apps/Package.swift index a974ba2..9f4e3a8 100644 --- a/apps/Package.swift +++ b/apps/Package.swift @@ -38,7 +38,11 @@ let package = Package( path: "shared/ServerGlassFFI", sources: ["generated/sg_ffi.swift"], linkerSettings: [ - .unsafeFlags(["-L\(workspaceTarget)", "-lsg_ffi"]), + // `-lsg_ffi` prefers the dylib Cargo also emits. Its install name contains the + // build machine's absolute path: an installed app then loads whatever a later + // build put there, including an incompatible UniFFI record layout. Link the + // archive explicitly so the bindings and their core ship as one executable. + .unsafeFlags(["\(workspaceTarget)/libsg_ffi.a"]), // russh's crypto backend and the keychain reach into these. .linkedFramework("Security"), .linkedFramework("CoreFoundation"), diff --git a/apps/android/app/build.gradle.kts b/apps/android/app/build.gradle.kts index 95f1a5c..3501edc 100644 --- a/apps/android/app/build.gradle.kts +++ b/apps/android/app/build.gradle.kts @@ -1,12 +1,11 @@ plugins { id("com.android.application") - id("org.jetbrains.kotlin.android") id("org.jetbrains.kotlin.plugin.compose") } android { namespace = "cloud.lazarev.serverglass" - compileSdk = 35 + compileSdk = 37 defaultConfig { applicationId = "cloud.lazarev.serverglass" @@ -42,18 +41,15 @@ android { sourceCompatibility = JavaVersion.VERSION_17 targetCompatibility = JavaVersion.VERSION_17 } - kotlinOptions { - jvmTarget = "17" - } buildFeatures { compose = true } sourceSets["main"].apply { // The Rust .so, placed here by cargo-ndk via scripts/build-android.sh. - jniLibs.srcDir("src/main/jniLibs") + jniLibs.directories.add("src/main/jniLibs") // The generated UniFFI bindings, kept out of the hand-written source tree. - java.srcDir("build/generated/uniffi") + kotlin.directories.add("build/generated/uniffi") } packaging { diff --git a/apps/android/app/src/main/kotlin/cloud/lazarev/serverglass/AddServer.kt b/apps/android/app/src/main/kotlin/cloud/lazarev/serverglass/AddServer.kt index 86a528f..9315c30 100644 --- a/apps/android/app/src/main/kotlin/cloud/lazarev/serverglass/AddServer.kt +++ b/apps/android/app/src/main/kotlin/cloud/lazarev/serverglass/AddServer.kt @@ -102,7 +102,7 @@ fun AddServerDialog( "key_text" -> keyText.isNotBlank() else -> secret.isNotBlank() } - val valid = address.isNotBlank() && user.isNotBlank() && port.toUShortOrNull() != null && + val valid = address.isNotBlank() && user.isNotBlank() && (port.toIntOrNull() ?: 0) in 1..65535 && (credentialGiven || isEditing) Dialog( @@ -128,14 +128,11 @@ fun AddServerDialog( color = Theme.secondary, fontSize = 13.sp, ) - if (!model.secretsAreHardwareBacked) { - // Computed since the vault landed and shown nowhere, which made a real weakening - // of the storage invisible: on a device whose Keystore is unavailable the password - // is kept like any ordinary app setting. + if (!model.secureStorageAvailable) { Spacer(Modifier.height(12.dp)) Text( - "This device's secure storage is unavailable, so a password or key is saved " + - "like any other app setting rather than encrypted. Consider a key file.", + "This device's secure storage is unavailable. Passwords and pasted keys " + + "cannot be saved. You can use a key file without a passphrase.", color = Theme.warn, fontSize = 11.5.sp, ) @@ -273,7 +270,7 @@ fun AddServerDialog( val path = if (authKind == "key") keyPath.trim() else null val key = if (authKind == "key_text") keyText.trim() else null - if (editing != null) { + val succeeded = if (editing != null) { model.updateHost( id = editing, address = address.trim(), @@ -300,7 +297,7 @@ fun AddServerDialog( trustOnFirstUse = trustOnFirstUse, ) } - onDismiss() + if (succeeded) onDismiss() }, enabled = valid, modifier = Modifier.weight(1f), diff --git a/apps/android/app/src/main/kotlin/cloud/lazarev/serverglass/HostStore.kt b/apps/android/app/src/main/kotlin/cloud/lazarev/serverglass/HostStore.kt index be8de37..ee057ef 100644 --- a/apps/android/app/src/main/kotlin/cloud/lazarev/serverglass/HostStore.kt +++ b/apps/android/app/src/main/kotlin/cloud/lazarev/serverglass/HostStore.kt @@ -38,37 +38,40 @@ class HostStore(context: Context) { private val config: SharedPreferences = context.getSharedPreferences("sg.hosts", Context.MODE_PRIVATE) - /** - * Falls back to plain preferences if the Keystore is unavailable. - * - * `EncryptedSharedPreferences` can fail on devices with a broken or wiped Keystore, and on some - * emulators. Refusing to run at all would be worse than storing a secret the way any ordinary - * app setting is stored — but the distinction is recorded so the UI can say which happened - * rather than implying protection it does not have. - */ - var secretsAreHardwareBacked: Boolean = true - private set - - private val secrets: SharedPreferences = try { + // A broken vault must never silently turn passwords and private keys into plain preferences. + private val secrets: SharedPreferences? = try { val masterKey = MasterKey.Builder(context) .setKeyScheme(MasterKey.KeyScheme.AES256_GCM) .build() - EncryptedSharedPreferences.create( + val encrypted = EncryptedSharedPreferences.create( context, "sg.secrets", masterKey, EncryptedSharedPreferences.PrefKeyEncryptionScheme.AES256_SIV, EncryptedSharedPreferences.PrefValueEncryptionScheme.AES256_GCM, ) + // Upgrade the old fallback only after a durable encrypted write. Until then the original + // remains recoverable; an unavailable Keystore must not erase someone's credentials. + val legacy = context.getSharedPreferences("sg.secrets.plain", Context.MODE_PRIVATE) + if (legacy.all.isNotEmpty()) { + val edit = encrypted.edit() + legacy.all.forEach { (key, value) -> + if (value is String && !encrypted.contains(key)) edit.putString(key, value) + } + check(edit.commit()) { "The sign-in details could not be moved to secure storage." } + check(legacy.edit().clear().commit()) { "The old sign-in storage could not be cleared." } + } + encrypted } catch (_: Exception) { - secretsAreHardwareBacked = false - context.getSharedPreferences("sg.secrets.plain", Context.MODE_PRIVATE) + null } - fun load(): List = decode(config.getString(KEY, null)) + val secureStorageAvailable: Boolean get() = secrets != null + + fun load(): List = decodeChecked(config.getString(KEY, null)) fun save(hosts: List) { - config.edit().putString(KEY, encode(hosts)).apply() + check(config.edit().putString(KEY, encode(hosts)).commit()) { "The server list could not be saved." } } /** Which secret. A host can have both — a pasted key *and* the passphrase protecting it. */ @@ -78,13 +81,20 @@ class HostStore(context: Context) { } fun secret(id: String, kind: Kind = Kind.PASSWORD): String? = - secrets.getString(id + kind.suffix, null)?.ifEmpty { null } - - fun setSecret(id: String, secret: String?, kind: Kind = Kind.PASSWORD) { - val account = id + kind.suffix - secrets.edit().apply { - if (secret.isNullOrEmpty()) remove(account) else putString(account, secret) - }.apply() + secrets?.getString(id + kind.suffix, null)?.ifEmpty { null } + + fun setSecrets(id: String, secret: String?, keyText: String?) { + if (secret == null && keyText == null) return + val vault = checkNotNull(secrets) { "This device's secure storage is unavailable. The sign-in details were not saved." } + val edit = vault.edit() + fun put(account: String, value: String?) { + if (value != null) { + if (value.isEmpty()) edit.remove(account) else edit.putString(account, value) + } + } + put(id, secret) + put(id + Kind.KEY_TEXT.suffix, keyText) + check(edit.commit()) { "The sign-in details could not be saved." } } /** @@ -102,7 +112,7 @@ class HostStore(context: Context) { /** Remove a host's stored record and its secret together. */ fun forget(id: String) { save(load().filterNot { it.id == id }) - secrets.edit().remove(id).remove(id + Kind.KEY_TEXT.suffix).apply() + secrets?.edit()?.remove(id)?.remove(id + Kind.KEY_TEXT.suffix)?.apply() } companion object { @@ -138,27 +148,30 @@ class HostStore(context: Context) { /** * Anything unreadable yields an empty list rather than a crash. * - * A record written by a future version, or a file truncated by a device losing power - * mid-write, must cost the user their list — not the ability to open the app at all. + * For previews and callers that only need a best-effort list. Persistence uses + * decodeChecked so a damaged inventory is reported and cannot be silently overwritten. */ - fun decode(raw: String?): List { + fun decode(raw: String?): List = runCatching { decodeChecked(raw) }.getOrDefault(emptyList()) + + internal fun decodeChecked(raw: String?): List { if (raw.isNullOrEmpty()) return emptyList() - return runCatching { - val array = JSONArray(raw) - (0 until array.length()).map { index -> - val o = array.getJSONObject(index) - SavedHost( - id = o.getString("id"), - address = o.getString("address"), - port = o.getInt("port").toUShort(), - user = o.getString("user"), - authKind = o.getString("authKind"), - keyPath = o.optString("keyPath").ifEmpty { null }, - hostKeyPolicy = o.getString("hostKeyPolicy"), - refreshMs = o.getLong("refreshMs").toULong(), - ) - } - }.getOrDefault(emptyList()) + val array = JSONArray(raw) + return (0 until array.length()).map { index -> + val o = array.getJSONObject(index) + val port = o.getInt("port") + val refresh = o.getLong("refreshMs") + require(port in 1..65535 && refresh > 0) { "Invalid saved connection settings" } + SavedHost( + id = o.getString("id"), + address = o.getString("address"), + port = port.toUShort(), + user = o.getString("user"), + authKind = o.getString("authKind"), + keyPath = o.optString("keyPath").ifEmpty { null }, + hostKeyPolicy = o.getString("hostKeyPolicy"), + refreshMs = refresh.toULong(), + ) + } } } } diff --git a/apps/android/app/src/main/kotlin/cloud/lazarev/serverglass/MainActivity.kt b/apps/android/app/src/main/kotlin/cloud/lazarev/serverglass/MainActivity.kt index 8d6c4ed..8e5ddb9 100644 --- a/apps/android/app/src/main/kotlin/cloud/lazarev/serverglass/MainActivity.kt +++ b/apps/android/app/src/main/kotlin/cloud/lazarev/serverglass/MainActivity.kt @@ -39,8 +39,11 @@ class MainActivity : ComponentActivity() { override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) enableEdgeToEdge() - val demoHost = intent?.getStringExtra("host") - val demoKey = intent?.getStringExtra("key") + // This activity is exported for the launcher. Production builds must not let another + // app turn test intent extras into automatic SSH connections using local key files. + val debugging = applicationInfo.flags and android.content.pm.ApplicationInfo.FLAG_DEBUGGABLE != 0 + val demoHost = if (debugging) intent?.getStringExtra("host") else null + val demoKey = if (debugging) intent?.getStringExtra("key") else null setContent { MaterialTheme( @@ -76,6 +79,18 @@ class MainActivity : ComponentActivity() { @OptIn(ExperimentalMaterial3WindowSizeClassApi::class) @Composable fun App(model: CoreModel = viewModel(), demoHost: String? = null, demoKey: String? = null) { + model.lastError?.let { message -> + androidx.compose.material3.AlertDialog( + onDismissRequest = { model.lastError = null }, + title = { androidx.compose.material3.Text("Could not complete that action") }, + text = { androidx.compose.material3.Text(message) }, + confirmButton = { + androidx.compose.material3.TextButton(onClick = { model.lastError = null }) { + androidx.compose.material3.Text("OK") + } + }, + ) + } LaunchedEffect(demoHost) { if (demoHost != null) model.addDemoHost(demoHost, demoKey) } diff --git a/apps/android/app/src/main/kotlin/cloud/lazarev/serverglass/Model.kt b/apps/android/app/src/main/kotlin/cloud/lazarev/serverglass/Model.kt index a235a23..5faf39a 100644 --- a/apps/android/app/src/main/kotlin/cloud/lazarev/serverglass/Model.kt +++ b/apps/android/app/src/main/kotlin/cloud/lazarev/serverglass/Model.kt @@ -8,6 +8,7 @@ import androidx.lifecycle.AndroidViewModel import androidx.lifecycle.viewModelScope import java.util.UUID import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.delay import kotlinx.coroutines.launch import kotlinx.coroutines.withContext @@ -51,11 +52,11 @@ class CoreModel(application: Application) : AndroidViewModel(application) { var showTechnical by mutableStateOf(false) private set - /** False when the Keystore was unavailable and secrets fell back to plain preferences. */ - val secretsAreHardwareBacked: Boolean get() = store.secretsAreHardwareBacked + val secureStorageAvailable: Boolean get() = store.secureStorageAvailable + var lastError by mutableStateOf(null) /** Secrets for hosts that were deliberately not saved. */ - private val ephemeralSecrets = mutableMapOf() + private val ephemeralSecrets = mutableMapOf>() init { viewModelScope.launch { @@ -70,7 +71,22 @@ class CoreModel(application: Application) : AndroidViewModel(application) { /** Reconnect everything added in a previous session. */ private fun restore() { - store.load().forEach { start(it) } + reportFailure { store.load().forEach { start(it) } } + } + + private inline fun reportFailure(action: () -> Unit): Boolean = try { + action() + true + } catch (error: CancellationException) { + throw error + } catch (error: Exception) { + lastError = error.message ?: "The server settings could not be saved or opened." + false + } + + override fun onCleared() { + core.destroy() + super.onCleared() } /** @@ -80,40 +96,45 @@ class CoreModel(application: Application) : AndroidViewModel(application) { * only for as long as building the config takes. */ private fun start(saved: HostStore.SavedHost, selectWhenReady: Boolean = false) { - val config = TargetConfig( - host = saved.address, - port = saved.port, - user = saved.user, - authKind = saved.authKind, - keyPath = saved.keyPath, - // A pasted key is key material, so it comes from the encrypted store rather than the - // record, exactly like the passphrase beside it. - keyText = store.secret(saved.id, HostStore.Kind.KEY_TEXT), - secret = if (ephemeralSecrets.containsKey(saved.id)) { - ephemeralSecrets[saved.id] - } else { - store.secret(saved.id) - }, - hostKeyPolicy = saved.hostKeyPolicy, - knownHostsPath = knownHosts, - refreshMs = saved.refreshMs, - ) + reportFailure { + val config = TargetConfig( + host = saved.address, + port = saved.port, + user = saved.user, + authKind = saved.authKind, + keyPath = saved.keyPath, + // A pasted key is key material, so it comes from the encrypted store rather than the + // record, exactly like the passphrase beside it. + keyText = if (ephemeralSecrets.containsKey(saved.id)) ephemeralSecrets[saved.id]?.second + else store.secret(saved.id, HostStore.Kind.KEY_TEXT), + secret = if (ephemeralSecrets.containsKey(saved.id)) { + ephemeralSecrets[saved.id]?.first + } else { + store.secret(saved.id) + }, + hostKeyPolicy = saved.hostKeyPolicy, + knownHostsPath = knownHosts, + refreshMs = saved.refreshMs, + ) - viewModelScope.launch { - val id = withContext(Dispatchers.IO) { - val id = core.addTarget(config) - core.start(id) - id + viewModelScope.launch { + reportFailure { + val id = withContext(Dispatchers.IO) { + val id = core.addTarget(config) + core.start(id) + id + } + // Deliberately does not select the new host. On a phone `selection` means "the user + // navigated into a server" and the detail screen replaces the list, so selecting + // automatically would drop someone into detail on launch with the list — and the Add + // button with it — out of reach. Two-pane layouts, where the list never leaves the + // screen, opt into a default selection themselves. + hosts = hosts + Host(id, "${saved.user}@${saved.address}", core.snapshot(id), saved.id) + // Only after an edit, so someone watching a host stays on it rather than being sent + // back to the list by their own change. + if (selectWhenReady) selection = id + } } - // Deliberately does not select the new host. On a phone `selection` means "the user - // navigated into a server" and the detail screen replaces the list, so selecting - // automatically would drop someone into detail on launch with the list — and the Add - // button with it — out of reach. Two-pane layouts, where the list never leaves the - // screen, opt into a default selection themselves. - hosts = hosts + Host(id, "${saved.user}@${saved.address}", core.snapshot(id), saved.id) - // Only after an edit, so someone watching a host stays on it rather than being sent - // back to the list by their own change. - if (selectWhenReady) selection = id } } @@ -129,7 +150,7 @@ class CoreModel(application: Application) : AndroidViewModel(application) { refreshMs: ULong = 1000UL, /** False for the development demo host, which would otherwise be saved on every launch. */ persist: Boolean = true, - ) { + ): Boolean { val saved = HostStore.SavedHost( id = UUID.randomUUID().toString(), address = address, @@ -143,29 +164,34 @@ class CoreModel(application: Application) : AndroidViewModel(application) { if (persist) { // The secret goes to the encrypted store and nowhere else; the record never carries it. - store.setSecret(saved.id, secret?.takeIf { it.isNotBlank() }) - store.setSecret(saved.id, keyText?.takeIf { it.isNotBlank() }, HostStore.Kind.KEY_TEXT) - store.save(store.load() + saved) + if (!reportFailure { + val stored = store.load() + store.setSecrets(saved.id, secret?.takeIf { it.isNotEmpty() }, keyText?.takeIf { it.isNotEmpty() }) + store.save(stored + saved) + }) return false } else { // Not saved, so the secret has to travel in memory rather than through the Keystore. - ephemeralSecrets[saved.id] = secret?.takeIf { it.isNotBlank() } + ephemeralSecrets[saved.id] = secret to keyText } start(saved) + return true } fun removeHost(id: String) { viewModelScope.launch { - withContext(Dispatchers.IO) { core.removeTarget(id) } - // Forget the stored record and its secret too, or the host returns on the next launch - // and its password is left behind in the Keystore. - hosts.firstOrNull { it.id == id }?.savedId?.takeIf { it.isNotEmpty() } - ?.let { savedId -> - store.forget(savedId) - ephemeralSecrets.remove(savedId) - } - hosts = hosts.filterNot { it.id == id } - if (selection == id) selection = hosts.firstOrNull()?.id + reportFailure { + withContext(Dispatchers.IO) { core.removeTarget(id) } + // Forget the stored record and its secret too, or the host returns on the next launch + // and its password is left behind in the Keystore. + hosts.firstOrNull { it.id == id }?.savedId?.takeIf { it.isNotEmpty() } + ?.let { savedId -> + store.forget(savedId) + ephemeralSecrets.remove(savedId) + } + hosts = hosts.filterNot { it.id == id } + if (selection == id) selection = hosts.firstOrNull()?.id + } } } @@ -193,11 +219,12 @@ class CoreModel(application: Application) : AndroidViewModel(application) { secret: String?, trustOnFirstUse: Boolean, refreshMs: ULong = 1000UL, - ) { - val savedId = hosts.firstOrNull { it.id == id }?.savedId ?: return - val stored = store.load().toMutableList() + ): Boolean { + val savedId = hosts.firstOrNull { it.id == id }?.savedId ?: return false + val stored = mutableListOf() + if (!reportFailure { stored.addAll(store.load()) }) return false val index = stored.indexOfFirst { it.id == savedId } - if (index < 0) return + if (index < 0) return false stored[index] = HostStore.SavedHost( id = savedId, @@ -209,25 +236,31 @@ class CoreModel(application: Application) : AndroidViewModel(application) { hostKeyPolicy = if (trustOnFirstUse) "accept_new" else "strict", refreshMs = refreshMs, ) - store.save(stored) - secret?.let { store.setSecret(savedId, it.takeIf(String::isNotBlank)) } - keyText?.let { store.setSecret(savedId, it.takeIf(String::isNotBlank), HostStore.Kind.KEY_TEXT) } + if (!reportFailure { + store.setSecrets(savedId, secret, keyText) + store.save(stored) + }) return false viewModelScope.launch { - // Not removeHost: that would also erase the record and its secrets, the very things - // being kept. - withContext(Dispatchers.IO) { core.removeTarget(id) } - val wasSelected = selection == id - hosts = hosts.filterNot { it.id == id } - if (wasSelected) selection = null - start(stored[index], selectWhenReady = wasSelected) + reportFailure { + // Not removeHost: that would also erase the record and its secrets, the very things + // being kept. + withContext(Dispatchers.IO) { core.removeTarget(id) } + val wasSelected = selection == id + hosts = hosts.filterNot { it.id == id } + if (wasSelected) selection = null + start(stored[index], selectWhenReady = wasSelected) + } } + return true } /** The saved record behind a live host, for populating an edit form. */ fun saved(id: String): HostStore.SavedHost? { val savedId = hosts.firstOrNull { it.id == id }?.savedId ?: return null - return store.load().firstOrNull { it.id == savedId } + var saved: HostStore.SavedHost? = null + reportFailure { saved = store.load().firstOrNull { it.id == savedId } } + return saved } /** diff --git a/apps/android/app/src/main/kotlin/cloud/lazarev/serverglass/Ui.kt b/apps/android/app/src/main/kotlin/cloud/lazarev/serverglass/Ui.kt index 6e6a0af..a1005ba 100644 --- a/apps/android/app/src/main/kotlin/cloud/lazarev/serverglass/Ui.kt +++ b/apps/android/app/src/main/kotlin/cloud/lazarev/serverglass/Ui.kt @@ -1,5 +1,7 @@ package cloud.lazarev.serverglass +import uniffi.sg_ffi.ConnectionState + import androidx.compose.foundation.Canvas import androidx.compose.foundation.background import androidx.compose.foundation.border @@ -76,12 +78,12 @@ object Theme { /// Slightly lifted from `panel`, for the larger simple-view cards. One flat surface colour at /// every size makes big cards read as empty space. val card = Color(0xFF19191D) - val border = Color(0x0FFFFFFF) + val border = Color(0x1AFFFFFF) val track = Color(0x14FFFFFF) val primary = Color(0xEBFFFFFF) - val secondary = Color(0x73FFFFFF) - val tertiary = Color(0x47FFFFFF) + val secondary = Color(0xB8FFFFFF) + val tertiary = Color(0x8CFFFFFF) val good = Color(0xFF59D68C) val warn = Color(0xFFFABF47) @@ -390,7 +392,7 @@ fun SimpleHostScreen( } item { HealthCard(snapshot.health, name) } - if (snapshot.simpleTiles.isEmpty()) { + if (snapshot.simpleTiles.isEmpty() && snapshot.health.level == "checking" && snapshot.state !is ConnectionState.Idle) { item { Box(Modifier.fillMaxWidth().height(180.dp), contentAlignment = Alignment.Center) { CircularProgressIndicator(color = Theme.secondary) diff --git a/apps/android/app/src/test/kotlin/cloud/lazarev/serverglass/HostStoreTest.kt b/apps/android/app/src/test/kotlin/cloud/lazarev/serverglass/HostStoreTest.kt index eb835c0..2c71b31 100644 --- a/apps/android/app/src/test/kotlin/cloud/lazarev/serverglass/HostStoreTest.kt +++ b/apps/android/app/src/test/kotlin/cloud/lazarev/serverglass/HostStoreTest.kt @@ -17,6 +17,18 @@ import org.junit.Test * Runs on the JVM: a test that needs an emulator is a test that does not run. */ class HostStoreTest { + @Test + fun `out of range ports cannot wrap to another server port`() { + val valid = HostStore.encode(listOf(sample())) + for (port in listOf(-1, 0, 65536, 67758)) { + assertTrue(HostStore.decode(valid.replace("2222", port.toString())).isEmpty()) + } + } + + @Test(expected = Exception::class) + fun `the application load path reports corruption instead of overwriting it`() { + HostStore.decodeChecked("{ damaged inventory") + } private fun sample(id: String = "abc-123") = HostStore.SavedHost( id = id, diff --git a/apps/android/build.gradle.kts b/apps/android/build.gradle.kts index c4be79c..159e2a8 100644 --- a/apps/android/build.gradle.kts +++ b/apps/android/build.gradle.kts @@ -1,5 +1,4 @@ plugins { id("com.android.application") version "9.3.2" apply false - id("org.jetbrains.kotlin.android") version "2.4.10" apply false id("org.jetbrains.kotlin.plugin.compose") version "2.4.10" apply false } diff --git a/apps/linux/Cargo.lock b/apps/linux/Cargo.lock index 934e1f7..75ce91a 100644 --- a/apps/linux/Cargo.lock +++ b/apps/linux/Cargo.lock @@ -275,7 +275,7 @@ checksum = "91e3bd0f4e25afa9cabc157908d14eeef9067d6448c49414d17b3fb55f0eadd0" dependencies = [ "bitflags", "cairo-sys-rs", - "glib 0.20.12", + "glib", "libc", ] @@ -285,7 +285,7 @@ version = "0.20.10" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "059cc746549898cbfd9a47754288e5a958756650ef4652bbb6c5f71a6bda4f8b" dependencies = [ - "glib-sys 0.20.10", + "glib-sys", "libc", "system-deps", ] @@ -710,7 +710,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d042b63efeb" dependencies = [ "libc", - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -892,7 +892,7 @@ checksum = "2fd242894c084f4beed508a56952750bce3e96e85eb68fdc153637daa163e10c" dependencies = [ "gdk-pixbuf-sys", "gio", - "glib 0.20.12", + "glib", "libc", ] @@ -902,9 +902,9 @@ version = "0.20.10" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5b34f3b580c988bd217e9543a2de59823fafae369d1a055555e5f95a8b130b96" dependencies = [ - "gio-sys 0.20.10", - "glib-sys 0.20.10", - "gobject-sys 0.20.10", + "gio-sys", + "glib-sys", + "gobject-sys", "libc", "system-deps", ] @@ -919,7 +919,7 @@ dependencies = [ "gdk-pixbuf", "gdk4-sys", "gio", - "glib 0.20.12", + "glib", "libc", "pango", ] @@ -932,9 +932,9 @@ checksum = "6f6eb95798e2b46f279cf59005daf297d5b69555428f185650d71974a910473a" dependencies = [ "cairo-sys-rs", "gdk-pixbuf-sys", - "gio-sys 0.20.10", - "glib-sys 0.20.10", - "gobject-sys 0.20.10", + "gio-sys", + "glib-sys", + "gobject-sys", "libc", "pango-sys", "pkg-config", @@ -995,8 +995,8 @@ dependencies = [ "futures-core", "futures-io", "futures-util", - "gio-sys 0.20.10", - "glib 0.20.12", + "gio-sys", + "glib", "libc", "pin-project-lite", "smallvec", @@ -1008,26 +1008,13 @@ version = "0.20.10" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "521e93a7e56fc89e84aea9a52cfc9436816a4b363b030260b699950ff1336c83" dependencies = [ - "glib-sys 0.20.10", - "gobject-sys 0.20.10", + "glib-sys", + "gobject-sys", "libc", "system-deps", "windows-sys 0.59.0", ] -[[package]] -name = "gio-sys" -version = "0.22.8" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "353fdc7da7cd16da916104b1e0e4e7de380ec9c8aaa20d4d742d66310ab4b0d5" -dependencies = [ - "glib-sys 0.22.8", - "gobject-sys 0.22.6", - "libc", - "system-deps", - "windows-sys 0.61.2", -] - [[package]] name = "glib" version = "0.20.12" @@ -1040,31 +1027,10 @@ dependencies = [ "futures-executor", "futures-task", "futures-util", - "gio-sys 0.20.10", - "glib-macros 0.20.12", - "glib-sys 0.20.10", - "gobject-sys 0.20.10", - "libc", - "memchr", - "smallvec", -] - -[[package]] -name = "glib" -version = "0.22.8" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ddbcf514bd1881fc1b960e4e52b4e82873f4da3bceddbd58d42827b508888100" -dependencies = [ - "bitflags", - "futures-channel", - "futures-core", - "futures-executor", - "futures-task", - "futures-util", - "gio-sys 0.22.8", - "glib-macros 0.22.6", - "glib-sys 0.22.8", - "gobject-sys 0.22.6", + "gio-sys", + "glib-macros", + "glib-sys", + "gobject-sys", "libc", "memchr", "smallvec", @@ -1083,18 +1049,6 @@ dependencies = [ "syn 2.0.119", ] -[[package]] -name = "glib-macros" -version = "0.22.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "506d23499707c7142898429757e8d9a3871d965239a2cb66dfa05052be6d6f19" -dependencies = [ - "heck", - "proc-macro2", - "quote", - "syn 2.0.119", -] - [[package]] name = "glib-sys" version = "0.20.10" @@ -1105,16 +1059,6 @@ dependencies = [ "system-deps", ] -[[package]] -name = "glib-sys" -version = "0.22.8" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "030967459f9f676851872c6304adea7825c6d462ec9b72554c733cf0c5952233" -dependencies = [ - "libc", - "system-deps", -] - [[package]] name = "glob" version = "0.3.4" @@ -1127,18 +1071,7 @@ version = "0.20.10" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ec9aca94bb73989e3cfdbf8f2e0f1f6da04db4d291c431f444838925c4c63eda" dependencies = [ - "glib-sys 0.20.10", - "libc", - "system-deps", -] - -[[package]] -name = "gobject-sys" -version = "0.22.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "22a861859b887a79cf461359c192c97a57d8fb0229dd291232e57aa11f6fa72c" -dependencies = [ - "glib-sys 0.22.8", + "glib-sys", "libc", "system-deps", ] @@ -1160,7 +1093,7 @@ version = "0.20.10" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6b86dfad7d14251c9acaf1de63bc8754b7e3b4e5b16777b6f5a748208fe9519b" dependencies = [ - "glib 0.20.12", + "glib", "graphene-sys", "libc", ] @@ -1171,7 +1104,7 @@ version = "0.20.10" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "df583a85ba2d5e15e1797e40d666057b28bc2f60a67c9c24145e6db2cc3861ea" dependencies = [ - "glib-sys 0.20.10", + "glib-sys", "libc", "pkg-config", "system-deps", @@ -1196,7 +1129,7 @@ checksum = "61f5e72f931c8c9f65fbfc89fe0ddc7746f147f822f127a53a9854666ac1f855" dependencies = [ "cairo-rs", "gdk4", - "glib 0.20.12", + "glib", "graphene-rs", "gsk4-sys", "libc", @@ -1211,8 +1144,8 @@ checksum = "755059de55fa6f85a46bde8caf03e2184c96bfda1f6206163c72fb0ea12436dc" dependencies = [ "cairo-sys-rs", "gdk4-sys", - "glib-sys 0.20.10", - "gobject-sys 0.20.10", + "glib-sys", + "gobject-sys", "graphene-sys", "libc", "pango-sys", @@ -1231,7 +1164,7 @@ dependencies = [ "gdk-pixbuf", "gdk4", "gio", - "glib 0.20.12", + "glib", "graphene-rs", "gsk4", "gtk4-macros", @@ -1261,9 +1194,9 @@ dependencies = [ "cairo-sys-rs", "gdk-pixbuf-sys", "gdk4-sys", - "gio-sys 0.20.10", - "glib-sys 0.20.10", - "gobject-sys 0.20.10", + "gio-sys", + "glib-sys", + "gobject-sys", "graphene-sys", "gsk4-sys", "libc", @@ -1438,7 +1371,7 @@ checksum = "500135d29c16aabf67baafd3e7741d48e8b8978ca98bac39e589165c8dc78191" dependencies = [ "gdk4", "gio", - "glib 0.20.12", + "glib", "gtk4", "libadwaita-sys", "libc", @@ -1452,9 +1385,9 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6680988058c2558baf3f548a370e4e78da3bf7f08469daa822ac414842c912db" dependencies = [ "gdk4-sys", - "gio-sys 0.20.10", - "glib-sys 0.20.10", - "gobject-sys 0.20.10", + "gio-sys", + "glib-sys", + "gobject-sys", "gtk4-sys", "libc", "pango-sys", @@ -1676,7 +1609,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6576b311f6df659397043a5fa8a021da8f72e34af180b44f7d57348de691ab5c" dependencies = [ "gio", - "glib 0.20.12", + "glib", "libc", "pango-sys", ] @@ -1687,8 +1620,8 @@ version = "0.20.10" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "186909673fc09be354555c302c0b3dcf753cd9fa08dcb8077fa663c80fb243fa" dependencies = [ - "glib-sys 0.20.10", - "gobject-sys 0.20.10", + "glib-sys", + "gobject-sys", "libc", "system-deps", ] @@ -2055,7 +1988,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys", - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -2231,7 +2164,7 @@ name = "sg-linux" version = "0.4.0" dependencies = [ "async-channel", - "glib 0.22.8", + "glib", "gtk4", "libadwaita", "qrcode", @@ -2514,7 +2447,7 @@ dependencies = [ "getrandom", "once_cell", "rustix", - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] diff --git a/apps/linux/Cargo.toml b/apps/linux/Cargo.toml index 870fb0b..9ec62b1 100644 --- a/apps/linux/Cargo.toml +++ b/apps/linux/Cargo.toml @@ -36,7 +36,8 @@ sg-sync = { path = "../../crates/sg-sync" } gtk4 = { version = "0.9", features = ["v4_10"] } adw = { package = "libadwaita", version = "0.7", features = ["v1_4"] } -glib = "0.22" +# Keep the direct GLib types aligned with GTK 0.9 / libadwaita 0.7. +glib = "0.20" serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" diff --git a/apps/linux/src/palette.rs b/apps/linux/src/palette.rs index 78766b6..24e0033 100644 --- a/apps/linux/src/palette.rs +++ b/apps/linux/src/palette.rs @@ -65,7 +65,7 @@ pub const STYLE: &str = " .sg-headline { font-size: 1.6rem; font-weight: 700; } .sg-tile-value { font-size: 1.5rem; font-weight: 700; font-feature-settings: 'tnum'; } -.sg-tile-name { font-size: 0.85rem; opacity: 0.7; letter-spacing: 0.04em; } +.sg-tile-name { font-size: 0.9rem; opacity: 0.85; letter-spacing: 0.04em; } .sg-tile-summary { font-size: 0.9rem; opacity: 0.8; } /* Monospaced, tabular figures: a changing value must not make the layout twitch. */ diff --git a/apps/linux/src/store.rs b/apps/linux/src/store.rs index 3596371..089fb5a 100644 --- a/apps/linux/src/store.rs +++ b/apps/linux/src/store.rs @@ -132,6 +132,9 @@ pub fn load(paths: &Paths) -> Result, String> { /// Written to a temporary file and renamed over the target: a crash or a full disk midway through /// must not leave a half-written inventory where a complete one used to be. pub fn save(paths: &Paths, hosts: &[SavedHost]) -> Result<(), String> { + // Never turn a failed restore into permanent loss when the next Add or pairing saves. + // The original file remains available for repair instead of being replaced by an empty list. + load(paths)?; if let Some(dir) = paths.config.parent() { std::fs::create_dir_all(dir) .map_err(|e| format!("Could not create {}: {e}", dir.display()))?; @@ -209,6 +212,8 @@ mod tests { // Returning Ok(vec![]) here would be indistinguishable from "the app forgot every server // you added", which is the one outcome a person must never be left to guess at. assert!(load(&paths).is_err()); + assert!(save(&paths, &[SavedHost::new("new-host")]).is_err()); + assert_eq!(std::fs::read(&paths.config).unwrap(), b"{ this is not json"); } #[test] diff --git a/apps/linux/src/views.rs b/apps/linux/src/views.rs index f3b5b9e..66d1d4f 100644 --- a/apps/linux/src/views.rs +++ b/apps/linux/src/views.rs @@ -80,11 +80,8 @@ impl SimpleView { detail.add_css_class("sg-tile-summary"); detail.set_xalign(0.0); detail.set_wrap(true); - // Two lines reserved whether or not both are needed, so "Barely working" and a full - // sentence about free space produce cards of the same height and the tiles below them do - // not jump on every refresh. - detail.set_lines(2); - detail.set_ellipsize(gtk4::pango::EllipsizeMode::End); + // Failure details contain the recovery step. Let them wrap instead of hiding it behind + // an ellipsis, especially with larger system text or a narrow window. card.append(&headline); card.append(&detail); diff --git a/apps/linux/src/window.rs b/apps/linux/src/window.rs index fa936e4..de2df5e 100644 --- a/apps/linux/src/window.rs +++ b/apps/linux/src/window.rs @@ -199,7 +199,7 @@ impl Window { dialogs::report_problem( &window, "Your server list could not be read", - &format!("{problem}\n\nNothing has been overwritten. Adding a server now would replace the file."), + &format!("{problem}\n\nNothing has been overwritten. Repair the file before saving changes."), ); }); } diff --git a/apps/shared/ServerGlassUI/AddHostSheet.swift b/apps/shared/ServerGlassUI/AddHostSheet.swift index f0de406..aa3f91b 100644 --- a/apps/shared/ServerGlassUI/AddHostSheet.swift +++ b/apps/shared/ServerGlassUI/AddHostSheet.swift @@ -33,8 +33,13 @@ public struct AddHostSheet: View { @State private var address = "" @State private var port = "22" - @State private var user = NSUserName() - @State private var authKind = "agent" + #if os(macOS) + @State private var user = NSUserName() + @State private var authKind = "agent" + #else + @State private var user = "root" + @State private var authKind = "password" + #endif @State private var keyPath = "" @State private var keyText = "" @State private var secret = "" @@ -47,7 +52,7 @@ public struct AddHostSheet: View { private var isValid: Bool { !address.trimmingCharacters(in: .whitespaces).isEmpty && !user.trimmingCharacters(in: .whitespaces).isEmpty - && UInt16(port) != nil + && (UInt16(port) ?? 0) > 0 && (authKind != "key" || !keyPath.isEmpty) // An edit keeps the key it already has, so an empty box is not an empty key. && (authKind != "key_text" || !keyText.isEmpty || isEditing) @@ -75,7 +80,9 @@ public struct AddHostSheet: View { // Agent first and default: with it, ServerGlass never holds key material. // It is also the only option a phone cannot use, which is why the paste // option exists directly below it. - Text("SSH agent").tag("agent") + #if os(macOS) + Text("SSH agent").tag("agent") + #endif Text("Private key").tag("key") Text("Paste a key").tag("key_text") Text("Password").tag("password") @@ -230,8 +237,9 @@ public struct AddHostSheet: View { let path = authKind == "key" ? keyPath : nil let key = authKind == "key_text" ? keyText : nil + let succeeded: Bool if let targetId = editingTargetId { - model.updateHost( + succeeded = model.updateHost( id: targetId, address: trimmedAddress, port: UInt16(port) ?? 22, @@ -246,7 +254,7 @@ public struct AddHostSheet: View { refreshMs: UInt64(refreshSeconds * 1000) ) } else { - model.addHost( + succeeded = model.addHost( address: trimmedAddress, port: UInt16(port) ?? 22, user: trimmedUser, @@ -258,7 +266,7 @@ public struct AddHostSheet: View { refreshMs: UInt64(refreshSeconds * 1000) ) } - dismiss() + if succeeded { dismiss() } } } diff --git a/apps/shared/ServerGlassUI/ContentView.swift b/apps/shared/ServerGlassUI/ContentView.swift index a6de0d4..c3c70e7 100644 --- a/apps/shared/ServerGlassUI/ContentView.swift +++ b/apps/shared/ServerGlassUI/ContentView.swift @@ -145,7 +145,10 @@ public struct ContentView: View { } } .onDelete { offsets in - for index in offsets { model.removeHost(id: model.hosts[index].id) } + // Resolve IDs before mutating the array. Deleting the first selected row + // shifts every later index and used to remove the wrong host or trap. + let ids = offsets.map { model.hosts[$0].id } + for id in ids { model.removeHost(id: id) } } } } @@ -220,7 +223,7 @@ struct Sidebar: View { // only appears once it means something. if model.hosts.count > 1 { Label("All hosts", systemImage: "square.grid.2x2") - .font(.system(size: 12, weight: .medium)) + .font(.body.weight(.medium)) .tag(Selection.statusID) } @@ -259,21 +262,21 @@ struct SidebarRow: View { Circle().fill(host.statusColor).frame(width: 7, height: 7) VStack(alignment: .leading, spacing: 1) { Text(host.snapshot.displayName.isEmpty ? host.address : host.snapshot.displayName) - .font(.system(size: 12, weight: .medium)) + .font(.body.weight(.medium)) .lineLimit(1) Text(host.statusText) - .font(.system(size: 9.5)) + .font(.caption) .foregroundStyle(Theme.secondary) .lineLimit(1) } Spacer(minLength: 4) - if let cpu = host.snapshot.gauge("cpu_usage") { + if host.isOnline, let cpu = host.snapshot.gauge("cpu_usage") { Text(String(format: "%.0f%%", cpu.value)) - .font(Theme.value(9.5, weight: .regular)) + .font(.caption.monospacedDigit()) .foregroundStyle(cpu.color) } } - .padding(.vertical, 2) + .padding(.vertical, 4) } } @@ -284,6 +287,9 @@ struct SidebarRow: View { struct StatusOverview: View { @EnvironmentObject private var model: CoreModel @Binding var showingAddHost: Bool + #if os(iOS) + @Environment(\.horizontalSizeClass) private var sizeClass + #endif var body: some View { ScrollView { @@ -296,8 +302,7 @@ struct StatusOverview: View { spacing: 12 ) { ForEach(model.hosts) { host in - HostCard(host: host) - .onTapGesture { model.selection = host.id } + hostLink(host) } } .padding(14) @@ -306,6 +311,25 @@ struct StatusOverview: View { .background(Theme.background) .navigationTitle("Status") } + + @ViewBuilder + private func hostLink(_ host: Host) -> some View { + #if os(iOS) + if sizeClass == .compact { + NavigationLink(value: host.id) { HostCard(host: host) } + .buttonStyle(.plain) + } else { + hostButton(host) + } + #else + hostButton(host) + #endif + } + + private func hostButton(_ host: Host) -> some View { + Button { model.selection = host.id } label: { HostCard(host: host) } + .buttonStyle(.plain) + } } /// One host as a card: the headline ring, then the three numbers that decide whether to look @@ -332,10 +356,15 @@ struct HostCard: View { } } - if snapshot.gauges.isEmpty { + if snapshot.gauges.isEmpty || !host.isOnline { HStack { - ProgressView().controlSize(.small) - Text(host.statusText) + if snapshot.health.level == "checking" { + ProgressView().controlSize(.small) + } else { + Image(systemName: HealthStyle.icon(snapshot.health.level)) + .foregroundStyle(Theme.level(snapshot.health.level)) + } + Text(snapshot.health.headline) .font(.system(size: 10.5)) .foregroundStyle(Theme.secondary) .lineLimit(2) @@ -403,10 +432,9 @@ struct MiniStat: View { .font(Theme.value(9.5, weight: .medium)) .foregroundStyle(Theme.primary) } - CapacityBar( - fraction: gauge.fraction ?? 0, - color: gauge.color, - height: 4) + if let fraction = gauge.fraction { + CapacityBar(fraction: fraction, color: gauge.color, height: 4) + } } } } diff --git a/apps/shared/ServerGlassUI/CoreModel.swift b/apps/shared/ServerGlassUI/CoreModel.swift index 7bc8324..efe5da6 100644 --- a/apps/shared/ServerGlassUI/CoreModel.swift +++ b/apps/shared/ServerGlassUI/CoreModel.swift @@ -100,6 +100,7 @@ public final class CoreModel: ObservableObject { } /// Register a host and start polling it. + @discardableResult public func addHost( address: String, port: UInt16, @@ -113,29 +114,32 @@ public final class CoreModel: ObservableObject { /// False for the development demo host, which would otherwise accumulate a duplicate /// saved record on every launch. persist: Bool = true - ) { + ) -> Bool { let saved = HostStore.SavedHost( address: address, port: port, user: user, authKind: authKind, keyPath: keyPath?.isEmpty == true ? nil : keyPath, hostKeyPolicy: hostKeyPolicy, refreshMs: refreshMs) if persist { + guard var stored = loadSavedHosts() else { return false } // Secrets go to the Keychain and nowhere else; the saved record never carries them. // A refusal is reported here rather than becoming a confusing connection error later. - if !Keychain.setSecret(secret, for: saved.id) - || !Keychain.setSecret(keyText, for: saved.id, kind: .keyText) + if (secret != nil && !Keychain.setSecret(secret, for: saved.id)) + || (keyText != nil && !Keychain.setSecret(keyText, for: saved.id, kind: .keyText)) { lastError = Self.keychainRefused + HostStore.forget(saved) + return false } - var stored = HostStore.load() stored.append(saved) HostStore.save(stored) } else { // Not persisted, so the secret has to travel with the config rather than the Keychain. - ephemeralSecrets[saved.id] = secret + ephemeralSecrets[saved.id] = (secret, keyText) } start(saved) + return true } /// Change a saved host and reconnect it with the new settings. @@ -149,6 +153,7 @@ public final class CoreModel: ObservableObject { /// an empty string meaning "clear it". An edit sheet cannot show an existing password, so /// treating a blank field as a deliberate erasure would silently discard the credential of /// anyone who edited a port number. + @discardableResult public func updateHost( id: String, address: String, @@ -160,24 +165,25 @@ public final class CoreModel: ObservableObject { secret: String?, hostKeyPolicy: String, refreshMs: UInt64 - ) { + ) -> Bool { guard let savedId = hosts.first(where: { $0.id == id })?.savedId, - let index = HostStore.load().firstIndex(where: { $0.id == savedId }) - else { return } + var stored = loadSavedHosts(), + let index = stored.firstIndex(where: { $0.id == savedId }) + else { return false } - var stored = HostStore.load() stored[index] = HostStore.SavedHost( id: savedId, address: address, port: port, user: user, authKind: authKind, keyPath: keyPath?.isEmpty == true ? nil : keyPath, hostKeyPolicy: hostKeyPolicy, refreshMs: refreshMs) - HostStore.save(stored) - if let secret, !Keychain.setSecret(secret, for: savedId) { lastError = Self.keychainRefused + return false } if let keyText, !Keychain.setSecret(keyText, for: savedId, kind: .keyText) { lastError = Self.keychainRefused + return false } + HostStore.save(stored) // Drop the live target and bring the record back up. Not `removeHost`, which would also // erase the record and its secrets — the very things being kept. @@ -188,12 +194,13 @@ public final class CoreModel: ObservableObject { let newId = start(stored[index]) if wasSelected { selection = newId } + return true } /// The saved record behind a live host, for populating an edit form. public func saved(for id: String) -> HostStore.SavedHost? { guard let savedId = hosts.first(where: { $0.id == id })?.savedId else { return nil } - return HostStore.load().first { $0.id == savedId } + return loadSavedHosts()?.first { $0.id == savedId } } static let keychainRefused = @@ -202,14 +209,15 @@ public final class CoreModel: ObservableObject { + "Keychain is unavailable; use an SSH agent or a key file instead." /// Secrets for hosts that were deliberately not saved. - private var ephemeralSecrets: [String: String?] = [:] + private var ephemeralSecrets: [String: (secret: String?, keyText: String?)] = [:] /// Bring a saved host up: hand its config to the core, start polling, and show it. @discardableResult private func start(_ saved: HostStore.SavedHost) -> String { var config = HostStore.config(for: saved) if let ephemeral = ephemeralSecrets[saved.id] { - config.secret = ephemeral + config.secret = ephemeral.secret + config.keyText = ephemeral.keyText } let id = core.addTarget(config: config) do { @@ -229,18 +237,27 @@ public final class CoreModel: ObservableObject { /// Reconnect everything that was added in a previous session. private func restore() { - for saved in HostStore.load() { + for saved in loadSavedHosts() ?? [] { start(saved) } } + private func loadSavedHosts() -> [HostStore.SavedHost]? { + do { return try HostStore.load() } + catch { + lastError = "The saved server list could not be read. It has been kept unchanged: \(error.localizedDescription)" + return nil + } + } + public func removeHost(id: String) { + guard var stored = loadSavedHosts() else { return } try? core.removeTarget(targetId: id) // Forget the stored record and its Keychain entry too, or removing a host from the list // would leave its password behind and the host itself would return on next launch. if let savedId = hosts.first(where: { $0.id == id })?.savedId { - var stored = HostStore.load() + ephemeralSecrets.removeValue(forKey: savedId) if let doomed = stored.first(where: { $0.id == savedId }) { HostStore.forget(doomed) } diff --git a/apps/shared/ServerGlassUI/DesignSystem.swift b/apps/shared/ServerGlassUI/DesignSystem.swift index d3f71a5..b29bb85 100644 --- a/apps/shared/ServerGlassUI/DesignSystem.swift +++ b/apps/shared/ServerGlassUI/DesignSystem.swift @@ -21,20 +21,19 @@ enum Theme { /// Slightly lifted from `panel`, for the simple view's larger cards. A single flat surface /// colour across every size makes big cards look like empty space. static let card = Color(red: 0.098, green: 0.098, blue: 0.114) - static let panelBorder = Color.white.opacity(0.06) + static let panelBorder = Color.white.opacity(0.10) static let inset = Color.black.opacity(0.25) static let track = Color.white.opacity(0.08) static let primary = Color.white.opacity(0.92) - static let secondary = Color.white.opacity(0.45) - static let tertiary = Color.white.opacity(0.28) + static let secondary = Color.white.opacity(0.72) + static let tertiary = Color.white.opacity(0.55) static let good = Color(red: 0.35, green: 0.84, blue: 0.55) static let warn = Color(red: 0.98, green: 0.75, blue: 0.28) static let bad = Color(red: 0.97, green: 0.44, blue: 0.44) static let info = Color(red: 0.42, green: 0.66, blue: 0.97) - /// Green below 60 %, amber to 85 %, red above — applied only where a fraction is real. /// Colour for a level the core assigned — to a host's health, a reading, or a process. /// /// The thresholds behind these levels used to live here *and* in Compose, and had already @@ -113,7 +112,7 @@ struct Panel: View { VStack(alignment: .leading, spacing: 10) { HStack(alignment: .firstTextBaseline, spacing: 6) { Text(title.uppercased()) - .font(Theme.label(9.5)) + .font(Theme.label(11)) .tracking(0.8) .foregroundStyle(Theme.secondary) if let subtitle { diff --git a/apps/shared/ServerGlassUI/HostStore.swift b/apps/shared/ServerGlassUI/HostStore.swift index 761a281..d280f99 100644 --- a/apps/shared/ServerGlassUI/HostStore.swift +++ b/apps/shared/ServerGlassUI/HostStore.swift @@ -48,9 +48,9 @@ public enum HostStore { } } - public static func load() -> [SavedHost] { + public static func load() throws -> [SavedHost] { guard let data = UserDefaults.standard.data(forKey: key) else { return [] } - return (try? JSONDecoder().decode([SavedHost].self, from: data)) ?? [] + return try JSONDecoder().decode([SavedHost].self, from: data) } public static func save(_ hosts: [SavedHost]) { @@ -125,18 +125,23 @@ enum Keychain { /// with no passcode restricts some accessibility classes — so it has to be reportable. @discardableResult static func setSecret(_ secret: String?, for id: String, kind: Kind = .password) -> Bool { - removeSecret(for: id, kind: kind) - guard let secret, !secret.isEmpty, let data = secret.data(using: .utf8) else { return true } + guard let secret, !secret.isEmpty, let data = secret.data(using: .utf8) else { + return removeSecret(for: id, kind: kind) + } - let query: [String: Any] = [ + var query: [String: Any] = [ kSecClass as String: kSecClassGenericPassword, kSecAttrService as String: service, kSecAttrAccount as String: account(id, kind), - kSecValueData as String: data, - // Available once the device has been unlocked, and never synced to another device or - // into a backup: a server password should not travel with an iCloud restore. - kSecAttrAccessible as String: kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly, ] + // Updating first preserves the working credential if the Keychain refuses the new one. + // Delete-then-add erased it before knowing whether the replacement could be saved. + let status = SecItemUpdate(query as CFDictionary, [kSecValueData as String: data] as CFDictionary) + guard status == errSecItemNotFound else { return status == errSecSuccess } + query[kSecValueData as String] = data + // Available once the device has been unlocked, and never synced to another device or + // into a backup: a server password should not travel with an iCloud restore. + query[kSecAttrAccessible as String] = kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly return SecItemAdd(query as CFDictionary, nil) == errSecSuccess } @@ -155,12 +160,14 @@ enum Keychain { return String(data: data, encoding: .utf8) } - static func removeSecret(for id: String, kind: Kind = .password) { + @discardableResult + static func removeSecret(for id: String, kind: Kind = .password) -> Bool { let query: [String: Any] = [ kSecClass as String: kSecClassGenericPassword, kSecAttrService as String: service, kSecAttrAccount as String: account(id, kind), ] - SecItemDelete(query as CFDictionary) + let status = SecItemDelete(query as CFDictionary) + return status == errSecSuccess || status == errSecItemNotFound } } diff --git a/apps/shared/ServerGlassUI/SimpleHostView.swift b/apps/shared/ServerGlassUI/SimpleHostView.swift index 45b8c6c..e0f547a 100644 --- a/apps/shared/ServerGlassUI/SimpleHostView.swift +++ b/apps/shared/ServerGlassUI/SimpleHostView.swift @@ -31,7 +31,7 @@ struct SimpleHostView: View { HealthCard(health: snapshot.health, name: displayName) if snapshot.simpleTiles.isEmpty { - loading + if snapshot.health.level == "checking" && snapshot.state != .idle { loading } } else { // Always one row. An adaptive grid wrapped 3 tiles as 2 + 1, leaving a // hole beside the last one; a fixed three columns with a ring sized to @@ -93,8 +93,7 @@ struct SimpleHostView: View { Spacer(minLength: 8) Text(String(format: "%.0f%%", process.cpuPercent)) .font(Theme.value(13, weight: .medium)) - .foregroundStyle( - process.cpuPercent >= 50 ? Theme.warn : Theme.secondary) + .foregroundStyle(Theme.level(process.severity)) } .padding(.vertical, 9) if process.pid != shown.last?.pid { @@ -239,8 +238,8 @@ struct SimpleTileCard: View { ZStack { let stroke = ring > 90 ? 8.0 : 6.0 - Circle().stroke(Theme.track, lineWidth: stroke) if let fraction = tile.fraction { + Circle().stroke(Theme.track, lineWidth: stroke) Circle() .trim(from: 0, to: fraction) .stroke(tint, style: StrokeStyle(lineWidth: stroke, lineCap: .round)) @@ -277,5 +276,8 @@ struct SimpleTileCard: View { .frame(maxWidth: .infinity) .background(Theme.card, in: RoundedRectangle(cornerRadius: 16)) .overlay(RoundedRectangle(cornerRadius: 16).strokeBorder(Theme.panelBorder)) + .accessibilityElement(children: .ignore) + .accessibilityLabel(tile.name) + .accessibilityValue("\(tile.valueText), \(tile.summary)") } } diff --git a/apps/shared/ServerGlassUITests/CoreBridgeTests.swift b/apps/shared/ServerGlassUITests/CoreBridgeTests.swift new file mode 100644 index 0000000..b073f3b --- /dev/null +++ b/apps/shared/ServerGlassUITests/CoreBridgeTests.swift @@ -0,0 +1,23 @@ +import ServerGlassFFI +import Testing + +/// Exercise the real boundary used during restore, not just the Swift configuration record. +struct CoreBridgeTests { + @Test("saved configurations cross the native boundary without trapping") + func restoresConfigurations() throws { + let core = ServerGlass() + for keyText: String? in [nil, "test key with Unicode: λ"] { + let config = TargetConfig( + host: "example.invalid", port: 2222, user: "test", authKind: "key_text", + keyPath: nil, keyText: keyText, secret: "test passphrase", + hostKeyPolicy: "strict", knownHostsPath: "/tmp/serverglass-bridge-known-hosts", + refreshMs: 1500) + let id = core.addTarget(config: config) + let snapshot = try core.snapshot(targetId: id) + #expect(snapshot.displayName == config.host) + #expect(snapshot.state == .idle) + try core.removeTarget(targetId: id) + } + #expect(core.targetIds().isEmpty) + } +} diff --git a/apps/shared/ServerGlassUITests/HostStoreTests.swift b/apps/shared/ServerGlassUITests/HostStoreTests.swift index d3bc96f..00649c9 100644 --- a/apps/shared/ServerGlassUITests/HostStoreTests.swift +++ b/apps/shared/ServerGlassUITests/HostStoreTests.swift @@ -14,6 +14,28 @@ import Testing /// removes what it wrote. @Suite(.serialized) struct HostStoreTests { + @Test("corrupt inventory is reported and left intact") + func corruptInventorySurvives() { + let key = "sg.hosts.v1" + let original = UserDefaults.standard.object(forKey: key) + defer { UserDefaults.standard.set(original, forKey: key) } + let damaged = Data("{ broken record".utf8) + UserDefaults.standard.set(damaged, forKey: key) + #expect(throws: (any Error).self) { try HostStore.load() } + #expect(UserDefaults.standard.data(forKey: key) == damaged) + } + + @Test("updating a secret replaces it without deleting its companion key") + func updateSecret() { + let id = UUID().uuidString + defer { Keychain.removeSecret(for: id); Keychain.removeSecret(for: id, kind: .keyText) } + guard Keychain.setSecret("old", for: id) else { return } + #expect(Keychain.setSecret("key", for: id, kind: .keyText)) + #expect(Keychain.setSecret("new", for: id)) + #expect(Keychain.secret(for: id) == "new") + #expect(Keychain.secret(for: id, kind: .keyText) == "key") + } + /// A host nobody else's test will collide with. private func sample(_ address: String = "10.0.0.9") -> HostStore.SavedHost { HostStore.SavedHost( @@ -22,7 +44,7 @@ struct HostStoreTests { } private func clear() { - for host in HostStore.load() { HostStore.forget(host) } + for host in (try? HostStore.load()) ?? [] { HostStore.forget(host) } HostStore.save([]) } @@ -34,7 +56,7 @@ struct HostStoreTests { let host = sample() HostStore.save([host]) - let loaded = HostStore.load() + let loaded = try HostStore.load() #expect(loaded.count == 1) #expect(loaded.first == host, "every field must survive, not just the address") } @@ -48,7 +70,7 @@ struct HostStoreTests { let host = sample() HostStore.save([host]) - #expect(HostStore.load().first?.id == host.id) + #expect(try HostStore.load().first?.id == host.id) } /// The record is what gets backed up and inspected. A password in it would be a password on diff --git a/apps/windows/ServerGlass.Core.Tests/CoreTests.cs b/apps/windows/ServerGlass.Core.Tests/CoreTests.cs index 268f173..efc3692 100644 --- a/apps/windows/ServerGlass.Core.Tests/CoreTests.cs +++ b/apps/windows/ServerGlass.Core.Tests/CoreTests.cs @@ -22,6 +22,29 @@ namespace ServerGlass.Core.Tests; /// public sealed class CoreTests { + [Fact] + public async Task Disposing_during_native_calls_does_not_free_an_in_use_handle() + { + var core = new ServerGlassCore(); + using var started = new ManualResetEventSlim(); + var calls = Task.Run(() => + { + for (var i = 0; i < 10_000; i++) + { + try + { + core.TargetIds(); + started.Set(); + } + catch (ObjectDisposedException) { return; } + } + }); + Assert.True(started.Wait(TimeSpan.FromSeconds(5))); + core.Dispose(); + await calls; + Assert.Throws(() => core.TargetIds()); + } + private static TargetConfig Sample(string host = "example.test") => new() { Host = host, diff --git a/apps/windows/ServerGlass.Core.Tests/HostStoreTests.cs b/apps/windows/ServerGlass.Core.Tests/HostStoreTests.cs index 0d0ff64..0cd51d5 100644 --- a/apps/windows/ServerGlass.Core.Tests/HostStoreTests.cs +++ b/apps/windows/ServerGlass.Core.Tests/HostStoreTests.cs @@ -175,6 +175,8 @@ public void A_corrupt_host_list_is_reported_rather_than_thrown_or_swallowed() Assert.Empty(store.Load()); Assert.NotNull(reported); + Assert.Throws(() => store.Save([Sample()])); + Assert.Equal("{ this is not the list }", File.ReadAllText(store.HostsPath)); } /// An empty store is an empty list, not a crash on first launch. diff --git a/apps/windows/ServerGlass.Core/Core.cs b/apps/windows/ServerGlass.Core/Core.cs index 38f9589..16a7ff8 100644 --- a/apps/windows/ServerGlass.Core/Core.cs +++ b/apps/windows/ServerGlass.Core/Core.cs @@ -48,13 +48,12 @@ public sealed unsafe class ServerGlassCore : IDisposable PropertyNameCaseInsensitive = true, }; - private void* _handle; - private bool _disposed; + private readonly CoreHandle _handle; public ServerGlassCore() { - _handle = Native.sg_core_new(); - if (_handle is null) + _handle = new CoreHandle(Native.sg_core_new()); + if (_handle.IsInvalid) { throw new SgException("internal", "The ServerGlass core could not be started.", false); } @@ -67,50 +66,58 @@ public ServerGlassCore() /// Register a host. Does not connect; call for that. public string AddTarget(TargetConfig config) { + using var lease = new CoreLease(_handle); var json = Utf8(JsonSerializer.Serialize(config, Json)); fixed (byte* p = json) { - return Read(Native.sg_add_target(Handle, p)); + return Read(Native.sg_add_target(lease.Pointer, p)); } } /// Connect and begin refreshing. Returns immediately; watch the snapshot for progress. public void Start(string targetId) { + using var lease = new CoreLease(_handle); fixed (byte* p = Utf8(targetId)) { - ReadVoid(Native.sg_start(Handle, p)); + ReadVoid(Native.sg_start(lease.Pointer, p)); } } /// Stop refreshing and drop the connection. public void Stop(string targetId) { + using var lease = new CoreLease(_handle); fixed (byte* p = Utf8(targetId)) { - ReadVoid(Native.sg_stop(Handle, p)); + ReadVoid(Native.sg_stop(lease.Pointer, p)); } } public void RemoveTarget(string targetId) { + using var lease = new CoreLease(_handle); fixed (byte* p = Utf8(targetId)) { - ReadVoid(Native.sg_remove_target(Handle, p)); + ReadVoid(Native.sg_remove_target(lease.Pointer, p)); } } /// The most recent completed refresh. Cheap enough to call on a display timer. public TargetSnapshot Snapshot(string targetId) { + using var lease = new CoreLease(_handle); fixed (byte* p = Utf8(targetId)) { - return Read(Native.sg_snapshot(Handle, p)); + return Read(Native.sg_snapshot(lease.Pointer, p)); } } - public IReadOnlyList TargetIds() => - Read>(Native.sg_target_ids(Handle)); + public IReadOnlyList TargetIds() + { + using var lease = new CoreLease(_handle); + return Read>(Native.sg_target_ids(lease.Pointer)); + } /// /// Run one command on the host and wait for what it printed. @@ -122,10 +129,11 @@ public IReadOnlyList TargetIds() => /// public CommandResult RunCommand(string targetId, string command) { + using var lease = new CoreLease(_handle); fixed (byte* id = Utf8(targetId)) fixed (byte* cmd = Utf8(command)) { - return Read(Native.sg_run_command(Handle, id, cmd)); + return Read(Native.sg_run_command(lease.Pointer, id, cmd)); } } @@ -138,9 +146,10 @@ public CommandResult RunCommand(string targetId, string command) public string Format(double value, string unitSuffix, bool binaryScaled) { + using var lease = new CoreLease(_handle); fixed (byte* unit = Utf8(unitSuffix)) { - return Read(Native.sg_format(Handle, value, unit, binaryScaled)); + return Read(Native.sg_format(lease.Pointer, value, unit, binaryScaled)); } } @@ -157,8 +166,11 @@ public string Format(MetricGauge gauge) => ? FormatDuration(gauge.Value) : Format(gauge.Value, gauge.UnitSuffix, gauge.BinaryScaled); - public string FormatDuration(double seconds) => - Read(Native.sg_format_duration(Handle, seconds)); + public string FormatDuration(double seconds) + { + using var lease = new CoreLease(_handle); + return Read(Native.sg_format_duration(lease.Pointer, seconds)); + } /// /// Normalise a series to 0-1 for a sparkline, oldest first. @@ -171,6 +183,7 @@ public string FormatDuration(double seconds) => /// public IReadOnlyList SparklinePoints(IReadOnlyList history) { + using var lease = new CoreLease(_handle); if (history.Count == 0) { return []; @@ -178,7 +191,7 @@ public IReadOnlyList SparklinePoints(IReadOnlyList history) fixed (byte* p = Utf8(JsonSerializer.Serialize(history, Json))) { - return Read>(Native.sg_sparkline_points(Handle, p)); + return Read>(Native.sg_sparkline_points(lease.Pointer, p)); } } @@ -196,9 +209,10 @@ public IReadOnlyList SparklinePoints(IReadOnlyList history) /// public ReceiverStarted StartReceiving(IReadOnlyList advertiseHosts) { + using var lease = new CoreLease(_handle); fixed (byte* p = Utf8(JsonSerializer.Serialize(advertiseHosts, Json))) { - return Read(Native.sg_start_receiving(Handle, p)); + return Read(Native.sg_start_receiving(lease.Pointer, p)); } } @@ -211,32 +225,44 @@ public ReceiverStarted StartReceiving(IReadOnlyList advertiseHosts) /// security of the exchange rests on that comparison happening first, which is why it is two /// calls and not one. /// - public string AwaitPairingConnection(ulong id) => - Read(Native.sg_receiver_await_connection(Handle, id)); + public string AwaitPairingConnection(ulong id) + { + using var lease = new CoreLease(_handle); + return Read(Native.sg_receiver_await_connection(lease.Pointer, id)); + } /// Take the transfer. Call only after the user confirmed the codes match. - public SyncBundle ReceivePairing(ulong id) => - Read(Native.sg_receiver_receive(Handle, id)); + public SyncBundle ReceivePairing(ulong id) + { + using var lease = new CoreLease(_handle); + return Read(Native.sg_receiver_receive(lease.Pointer, id)); + } /// Connect to a pairing code from the other device. Nothing is sent yet. public SenderConnected ScanPairingCode(string code) { + using var lease = new CoreLease(_handle); fixed (byte* p = Utf8(code)) { - return Read(Native.sg_scan_pairing_code(Handle, p)); + return Read(Native.sg_scan_pairing_code(lease.Pointer, p)); } } /// Send the bundle. Call only after the user confirmed the codes match. public void SendPairing(ulong id, SyncBundle bundle) { + using var lease = new CoreLease(_handle); fixed (byte* p = Utf8(JsonSerializer.Serialize(bundle, Json))) { - ReadVoid(Native.sg_sender_send(Handle, id, p)); + ReadVoid(Native.sg_sender_send(lease.Pointer, id, p)); } } - public void ForgetPairing(ulong id) => Native.sg_pairing_forget(Handle, id); + public void ForgetPairing(ulong id) + { + using var lease = new CoreLease(_handle); + Native.sg_pairing_forget(lease.Pointer, id); + } /// /// Apply a received bundle to what this device already has. @@ -248,10 +274,11 @@ public void SendPairing(ulong id, SyncBundle bundle) /// public MergeResult MergeBundle(SyncBundle existing, SyncBundle incoming) { + using var lease = new CoreLease(_handle); fixed (byte* a = Utf8(JsonSerializer.Serialize(existing, Json))) fixed (byte* b = Utf8(JsonSerializer.Serialize(incoming, Json))) { - return Read(Native.sg_merge_bundle(Handle, a, b)); + return Read(Native.sg_merge_bundle(lease.Pointer, a, b)); } } @@ -259,10 +286,37 @@ public MergeResult MergeBundle(SyncBundle existing, SyncBundle incoming) // Plumbing // ----------------------------------------------------------------------------------------- - private void* Handle => - _disposed - ? throw new ObjectDisposedException(nameof(ServerGlassCore)) - : _handle; + /// SafeHandle keeps the Rust allocation alive until every in-flight native call returns. + /// Checking a boolean before a call is insufficient: closing the window can free the core + /// between that check and an SSH command or snapshot running on a worker thread. + private sealed class CoreHandle : SafeHandle + { + public CoreHandle(void* pointer) : base(IntPtr.Zero, ownsHandle: true) => SetHandle((IntPtr)pointer); + public override bool IsInvalid => handle == IntPtr.Zero; + protected override bool ReleaseHandle() + { + Native.sg_core_free((void*)handle); + return true; + } + } + + private sealed class CoreLease : IDisposable + { + private readonly CoreHandle _owner; + private bool _acquired; + public CoreLease(CoreHandle owner) + { + _owner = owner; + owner.DangerousAddRef(ref _acquired); + } + public void* Pointer => (void*)_owner.DangerousGetHandle(); + public void Dispose() + { + if (!_acquired) return; + _acquired = false; + _owner.DangerousRelease(); + } + } /// A string as NUL-terminated UTF-8, ready to pin. private static byte[] Utf8(string text) @@ -328,17 +382,5 @@ private static void ThrowIfError(JsonDocument document) error.TryGetProperty("recoverable", out var recoverable) && recoverable.GetBoolean()); } - public void Dispose() - { - if (_disposed) - { - return; - } - - _disposed = true; - var handle = _handle; - _handle = null; - // Drops every target's poll loop with it. - Native.sg_core_free(handle); - } + public void Dispose() => _handle.Dispose(); } diff --git a/apps/windows/ServerGlass.Core/HostStore.cs b/apps/windows/ServerGlass.Core/HostStore.cs index f5ee0eb..95b1a7c 100644 --- a/apps/windows/ServerGlass.Core/HostStore.cs +++ b/apps/windows/ServerGlass.Core/HostStore.cs @@ -51,6 +51,7 @@ public sealed class HostStore }; private readonly ISecretStore _secrets; + private bool _loadFailed; public HostStore(ISecretStore secrets, string? directory = null) { @@ -83,18 +84,22 @@ public IReadOnlyList Load() { if (!File.Exists(HostsPath)) { + _loadFailed = false; return []; } try { - return JsonSerializer.Deserialize>(File.ReadAllText(HostsPath), FileJson) - ?? []; + var hosts = JsonSerializer.Deserialize>(File.ReadAllText(HostsPath), FileJson) + ?? throw new JsonException("The server list is null."); + _loadFailed = false; + return hosts; } - catch (Exception error) when (error is JsonException or IOException) + catch (Exception error) when (error is JsonException or IOException or UnauthorizedAccessException) { // A corrupt or unreadable list must not stop the app from starting. It is reported // rather than swallowed, because a discarded error is a silent failure. + _loadFailed = true; LoadFailed?.Invoke(this, error); return []; } @@ -105,6 +110,8 @@ public IReadOnlyList Load() public void Save(IReadOnlyList hosts) { + if (_loadFailed) + throw new InvalidOperationException("The unreadable server list has been kept unchanged. Repair it before saving changes."); System.IO.Directory.CreateDirectory(Directory); // Written to a temporary file and moved into place, so a crash mid-write cannot leave a // half-written list where a complete one used to be. diff --git a/apps/windows/ServerGlass/CoreModel.cs b/apps/windows/ServerGlass/CoreModel.cs index 746f463..a7c3238 100644 --- a/apps/windows/ServerGlass/CoreModel.cs +++ b/apps/windows/ServerGlass/CoreModel.cs @@ -31,6 +31,7 @@ public TargetSnapshot Snapshot Raise(); Raise(nameof(Title)); Raise(nameof(Subtitle)); + Raise(nameof(StatusLevel)); } } @@ -213,27 +214,39 @@ .. System.Net.NetworkInformation.NetworkInterface.GetAllNetworkInterfaces() private async Task PollForever(CancellationToken token) { - using var timer = new PeriodicTimer(TimeSpan.FromMilliseconds(1000)); - while (await timer.WaitForNextTickAsync(token).ConfigureAwait(false)) + try { - // The list is only mutated on the UI thread, so a copy is enough to iterate safely. - var hosts = Hosts.ToArray(); - foreach (var host in hosts) + using var timer = new PeriodicTimer(TimeSpan.FromMilliseconds(1000)); + while (await timer.WaitForNextTickAsync(token).ConfigureAwait(false)) { - TargetSnapshot snapshot; - try + // Copy on the thread that owns the ObservableCollection. ToArray on a worker still + // enumerates concurrently with Add/Remove and used to silently kill this poll task. + var copy = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + if (!_dispatcher.TryEnqueue(() => copy.TrySetResult(Hosts.ToArray()))) return; + var hosts = await copy.Task.WaitAsync(token).ConfigureAwait(false); + foreach (var host in hosts) { - snapshot = _core.Snapshot(host.TargetId); + TargetSnapshot snapshot; + try + { + snapshot = _core.Snapshot(host.TargetId); + } + catch (SgException) + { + // The target was removed between the copy and the call. Nothing to draw. + continue; + } + + _dispatcher.TryEnqueue(() => + { + if (!token.IsCancellationRequested && Hosts.Contains(host)) host.Snapshot = snapshot; + }); } - catch (SgException) - { - // The target was removed between the copy and the call. Nothing to draw. - continue; - } - - _dispatcher.TryEnqueue(() => host.Snapshot = snapshot); } } + catch (OperationCanceledException) when (token.IsCancellationRequested) { } + catch (ObjectDisposedException) when (token.IsCancellationRequested) { } + catch (Exception error) { Report($"Live updates stopped: {error.Message}"); } } public void Dispose() diff --git a/apps/windows/ServerGlass/Theme.cs b/apps/windows/ServerGlass/Theme.cs index 96c1810..ac7d07f 100644 --- a/apps/windows/ServerGlass/Theme.cs +++ b/apps/windows/ServerGlass/Theme.cs @@ -36,13 +36,13 @@ private static Color White(double opacity) => public static readonly Color Background = Rgb(11, 11, 13); public static readonly Color Panel = Rgb(21, 21, 24); public static readonly Color Card = Rgb(25, 25, 29); - public static readonly Color PanelBorder = White(0.06); + public static readonly Color PanelBorder = White(0.10); public static readonly Color Track = White(0.08); // Text. public static readonly Color Primary = White(0.92); - public static readonly Color Secondary = White(0.45); - public static readonly Color Tertiary = White(0.28); + public static readonly Color Secondary = White(0.72); + public static readonly Color Tertiary = White(0.55); // Levels. public static readonly Color Good = Rgb(89, 214, 140); diff --git a/apps/windows/ServerGlass/Views/PlainView.cs b/apps/windows/ServerGlass/Views/PlainView.cs index d6c28a8..4cc25ac 100644 --- a/apps/windows/ServerGlass/Views/PlainView.cs +++ b/apps/windows/ServerGlass/Views/PlainView.cs @@ -47,7 +47,7 @@ public void Update(TargetSnapshot snapshot, string address) _root.Children.Add(HealthCard(snapshot.Health, string.IsNullOrEmpty(snapshot.DisplayName) ? address : snapshot.DisplayName)); - if (snapshot.SimpleTiles.Count == 0) + if (snapshot.SimpleTiles.Count == 0 && snapshot.Health.Level == "checking" && snapshot.State.Kind != "idle") { _root.Children.Add(new TextBlock { @@ -266,7 +266,7 @@ private FrameworkElement Busiest(TargetSnapshot snapshot) var percent = Widgets.Value( $"{Math.Round(process.CpuPercent):0}%", 13, - process.CpuPercent >= 50 ? Theme.Warn : Theme.Secondary); + Theme.Level(process.Severity)); Grid.SetColumn(percent, 1); row.Children.Add(command); diff --git a/crates/sg-core/src/rate.rs b/crates/sg-core/src/rate.rs index 74d0c83..3d0dd04 100644 --- a/crates/sg-core/src/rate.rs +++ b/crates/sg-core/src/rate.rs @@ -49,6 +49,12 @@ impl RateEngine { let by_id: HashMap<&SeriesId, &SeriesDescriptor> = descriptors.iter().map(|d| (&d.id, d)).collect(); + // Process IDs and interface names churn for the entire lifetime of a connection. The + // chart store was bounded but these baselines were not, and a reused PID inherited the + // previous process's CPU counter. An absent series must establish a new baseline. + self.previous + .retain(|id, _| by_id.get(id).is_some_and(|d| d.kind == SeriesKind::Counter)); + let mut out = Vec::with_capacity(samples.len()); for sample in samples { let Some(descriptor) = by_id.get(&sample.series) else { @@ -73,7 +79,7 @@ impl RateEngine { ); let Some(previous) = previous else { continue }; - let elapsed_ms = sample.at_ms - previous.at_ms; + let elapsed_ms = sample.at_ms.saturating_sub(previous.at_ms); if elapsed_ms <= 0 || raw < previous.raw { continue; } @@ -124,6 +130,31 @@ mod tests { Sample::new(descriptor.id.clone(), at_ms, value) } + #[test] + fn churn_discards_old_counters_and_reused_ids_start_fresh() { + let mut engine = RateEngine::new(); + let first = counter("process-0", Unit::Percent, 1.0); + for index in 0..1_000 { + let d = counter(&format!("process-{index}"), Unit::Percent, 1.0); + let out = engine.process( + std::slice::from_ref(&d), + vec![sample(&d, index * 1000, 100)], + ); + assert!(out.is_empty()); + assert_eq!(engine.tracked(), 1); + } + let out = engine.process( + std::slice::from_ref(&first), + vec![sample(&first, 1_000_000, 200)], + ); + assert!( + out.is_empty(), + "a reused process ID inherited an old counter" + ); + engine.process(&[], vec![]); + assert_eq!(engine.tracked(), 0); + } + #[test] fn a_single_reading_is_not_a_rate() { let rx = counter("rx", Unit::Bytes, 1.0); diff --git a/crates/sg-core/src/runtime.rs b/crates/sg-core/src/runtime.rs index 19e7da9..b989071 100644 --- a/crates/sg-core/src/runtime.rs +++ b/crates/sg-core/src/runtime.rs @@ -131,7 +131,13 @@ impl TargetRuntime { } }; - let responses = session.batch(&sg_transport::probe::requests()).await?; + let responses = match session.batch(&sg_transport::probe::requests()).await { + Ok(responses) => responses, + Err(error) => { + self.on_disconnect(&error); + return Err(error); + } + }; let caps = sg_transport::probe::parse(&responses); // Prefer the host's own idea of its name over whatever the user typed into the address @@ -192,6 +198,7 @@ impl TargetRuntime { tick.descriptors.clone(), &tick.samples, ); + self.store.retain_series(&tick.descriptors); // Prune anything that stopped being reported — a stopped container must leave the // dashboard rather than sitting there with its last reading frozen. let mut present = tick.entity_ids(); diff --git a/crates/sg-core/src/store.rs b/crates/sg-core/src/store.rs index 9eb450b..cee7a44 100644 --- a/crates/sg-core/src/store.rs +++ b/crates/sg-core/src/store.rs @@ -69,7 +69,7 @@ impl LiveStore { } for sample in samples { match sample.value.as_f64() { - Some(value) => { + Some(value) if value.is_finite() => { let series = self.history.entry(sample.series.clone()).or_default(); series.push_back(Point { at_ms: sample.at_ms, @@ -79,7 +79,7 @@ impl LiveStore { series.pop_front(); } } - None => { + _ => { if let sg_model::Value::Text(text) = &sample.value { self.text.insert(sample.series.clone(), text.clone()); } @@ -88,6 +88,15 @@ impl LiveStore { } } + /// Forget series absent from a complete tick, even if their host entity is still present. + /// Swap can be disabled or a collector removed without removing the host itself. + pub fn retain_series(&mut self, descriptors: &[SeriesDescriptor]) { + let present: std::collections::HashSet<_> = descriptors.iter().map(|d| &d.id).collect(); + self.descriptors.retain(|id, _| present.contains(id)); + self.history.retain(|id, _| present.contains(id)); + self.text.retain(|id, _| present.contains(id)); + } + /// Drop entities absent from `present`, along with their series. /// /// Without this, a container that exits stays on the dashboard forever with its last reading @@ -209,6 +218,40 @@ mod tests { Entity::host("web-01") } + #[test] + fn disappearing_metrics_are_removed_even_when_the_host_remains() { + let mut store = LiveStore::default(); + let swap = descriptor(&host().id, "swap"); + let memory = descriptor(&host().id, "memory"); + store.ingest( + vec![host()], + vec![swap.clone(), memory.clone()], + &[ + Sample::new(swap.id.clone(), 0, 40.0), + Sample::new(memory.id.clone(), 0, 50.0), + ], + ); + store.retain_series(std::slice::from_ref(&memory)); + assert!(store.entity(&host().id).is_some()); + assert!(store.descriptor(&swap.id).is_none()); + assert!(store.latest(&swap.id).is_none()); + assert_eq!(store.point_count(), 1); + } + + #[test] + fn non_finite_readings_do_not_reach_charts_or_json() { + let mut store = LiveStore::default(); + let d = descriptor(&host().id, "cpu"); + for value in [f64::NAN, f64::INFINITY, f64::NEG_INFINITY] { + store.ingest( + vec![host()], + vec![d.clone()], + &[Sample::new(d.id.clone(), 0, value)], + ); + } + assert_eq!(store.point_count(), 0); + } + #[test] fn keeps_readings_in_arrival_order() { let mut store = LiveStore::default(); diff --git a/crates/sg-ffi/src/lib.rs b/crates/sg-ffi/src/lib.rs index 332ba5e..93c2401 100644 --- a/crates/sg-ffi/src/lib.rs +++ b/crates/sg-ffi/src/lib.rs @@ -19,6 +19,7 @@ mod sync; mod view; use std::collections::HashMap; +use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::{Arc, Mutex, RwLock}; use sg_core::{default_sources, TargetRuntime, TargetState}; @@ -61,16 +62,18 @@ type CommandJob = ( struct Target { config: TargetConfig, snapshot: Arc>, - /// Dropping this aborts the poll loop. - task: Mutex>>, + task: Mutex>, + generation: AtomicU64, +} + +struct RunningTask { + handle: tokio::task::JoinHandle<()>, /// Commands waiting to run on the poll loop's session. /// /// A queue rather than a second connection: the poll loop owns the session, and opening /// another one per command would authenticate again, double the connections a host sees, and /// give the command a different environment from the one the readings come from. commands: tokio::sync::mpsc::UnboundedSender, - /// Held so the receiver survives being handed to each new poll-loop attempt. - command_inbox: tokio::sync::Mutex>, } /// What one command printed, and how it ended. @@ -125,16 +128,13 @@ impl ServerGlass { ConnectionState::Idle, ))); - let (commands, command_inbox) = tokio::sync::mpsc::unbounded_channel(); - self.targets.write().expect("targets lock").insert( id.clone(), Arc::new(Target { config, snapshot, task: Mutex::new(None), - commands, - command_inbox: tokio::sync::Mutex::new(command_inbox), + generation: AtomicU64::new(0), }), ); id @@ -145,14 +145,23 @@ impl ServerGlass { let target = self.target(&target_id)?; let mut slot = target.task.lock().expect("task lock"); - if slot.as_ref().is_some_and(|t| !t.is_finished()) { + if slot.as_ref().is_some_and(|t| !t.handle.is_finished()) { return Ok(()); } - let handle = self - .runtime - .spawn(poll_loop(target_id, Arc::clone(&target))); - *slot = Some(handle); + // Each run owns its inbox. Stopping drops queued commands instead of carrying them into + // a later start, and immediately publishes the state the UI should display. + let (commands, inbox) = tokio::sync::mpsc::unbounded_channel(); + let generation = { + let mut snapshot = target.snapshot.write().expect("snapshot lock"); + let generation = target.generation.fetch_add(1, Ordering::Relaxed) + 1; + snapshot.set_state(ConnectionState::Connecting); + generation + }; + let handle = + self.runtime + .spawn(poll_loop(target_id, Arc::clone(&target), inbox, generation)); + *slot = Some(RunningTask { handle, commands }); Ok(()) } @@ -184,25 +193,28 @@ impl ServerGlass { // the channel and run on reconnect — minutes later, with nobody watching, having been // typed against a machine in a different state. `systemctl restart` firing five minutes // after someone gave up on it is not a delay, it is a surprise. - if !matches!( - target.snapshot.read().expect("snapshot lock").state, - ConnectionState::Online - ) { - return Err(SgError::Connection { + let (reply, answer) = tokio::sync::oneshot::channel(); + { + let task = target.task.lock().expect("task lock"); + let snapshot = target.snapshot.read().expect("snapshot lock"); + let disconnected = || SgError::Connection { detail: "This server is not connected, so the command was not run.".into(), recoverable: true, - }); + }; + let task = task + .as_ref() + .filter(|t| !t.handle.is_finished()) + .ok_or_else(disconnected)?; + if snapshot.state != ConnectionState::Online { + return Err(disconnected()); + } + // Hold the state read lock through enqueueing, so a disconnect cannot drain the + // inbox between the online check and the send. + task.commands + .send((command, reply)) + .map_err(|_| disconnected())?; } - let (reply, answer) = tokio::sync::oneshot::channel(); - target - .commands - .send((command, reply)) - .map_err(|_| SgError::Connection { - detail: "This server is not connected.".into(), - recoverable: true, - })?; - // A bounded wait: a command that never returns must not leave the caller's thread parked // for the life of the app. Sixty seconds is long enough for a package list and short // enough to notice. @@ -231,11 +243,15 @@ impl ServerGlass { /// Stop refreshing and drop the connection. pub fn stop(&self, target_id: String) -> Result<(), SgError> { let target = self.target(&target_id)?; - if let Some(handle) = target.task.lock().expect("task lock").take() { - handle.abort(); + let mut task = target.task.lock().expect("task lock"); + if let Some(task) = task.take() { + task.handle.abort(); } let mut snapshot = target.snapshot.write().expect("snapshot lock"); - snapshot.state = ConnectionState::Idle; + // abort() is asynchronous. A tick already building its view model must not overwrite + // Idle, or a new run's state, after stop has returned. + target.generation.fetch_add(1, Ordering::Relaxed); + snapshot.set_state(ConnectionState::Idle); Ok(()) } @@ -289,10 +305,17 @@ impl ServerGlass { } /// Connect, then refresh forever, republishing a snapshot after each tick. -async fn poll_loop(target_id: String, target: Arc) { +async fn poll_loop( + target_id: String, + target: Arc, + mut inbox: tokio::sync::mpsc::UnboundedReceiver, + generation: u64, +) { let publish = |state: ConnectionState| { let mut snapshot = target.snapshot.write().expect("snapshot lock"); - snapshot.state = state; + if target.generation.load(Ordering::Relaxed) == generation { + snapshot.set_state(state); + } }; let interval = std::time::Duration::from_millis(target.config.refresh_ms.clamp(250, 60_000)); @@ -338,13 +361,16 @@ async fn poll_loop(target_id: String, target: Arc) { // complete and stable the moment it appears. let mut published_first = false; - loop { + 'refresh: loop { let tick_started = tokio::time::Instant::now(); match runtime.tick().await { Ok(tick) => { if published_first { let snapshot = build_snapshot(&target_id, &runtime, &tick); - *target.snapshot.write().expect("snapshot lock") = snapshot; + let mut current = target.snapshot.write().expect("snapshot lock"); + if target.generation.load(Ordering::Relaxed) == generation { + *current = snapshot; + } } else { published_first = true; publish(ConnectionState::Online); @@ -368,12 +394,13 @@ async fn poll_loop(target_id: String, target: Arc) { // just pressed Return should not wait out a ten-second refresh interval, and a slow // command must not delay the readings any longer than it has to. loop { - let mut inbox = target.command_inbox.lock().await; tokio::select! { _ = &mut until_next => break, job = inbox.recv() => { let Some((command, reply)) = job else { break }; - drop(inbox); + // A caller that timed out has withdrawn its queued command. Executing it + // now could apply an action after the user was told it did not complete. + if reply.is_closed() { continue; } let answer = runtime .run_command(&command) .await @@ -391,7 +418,7 @@ async fn poll_loop(target_id: String, target: Arc) { // `run_command` has already moved the runtime into Reconnecting or // Failed; surface it and rebuild the session. publish(ConnectionState::from(runtime.state())); - break; + break 'refresh; } } } @@ -401,7 +428,6 @@ async fn poll_loop(target_id: String, target: Arc) { // The session is gone. Anything still queued was typed against it, so it is refused here // rather than carried into the next one. { - let mut inbox = target.command_inbox.lock().await; while let Ok((_, reply)) = inbox.try_recv() { let _ = reply.send(Err( "The connection dropped before the command could run.".to_string() @@ -449,6 +475,12 @@ fn build_snapshot( }; let state = ConnectionState::from(runtime.state()); + let mut all = gauges.clone(); + all.extend( + detail_groups + .iter() + .flat_map(|group| group.gauges.iter().cloned()), + ); TargetSnapshot { target_id: target_id.to_string(), state: state.clone(), @@ -457,12 +489,8 @@ fn build_snapshot( kernel: caps.map(|c| c.kernel.clone()).unwrap_or_default(), arch: caps.map(|c| c.arch.clone()).unwrap_or_default(), cpu_count: caps.map(|c| c.cpu_count).unwrap_or(0), - health: plain::assess(&state, &gauges, !gauges.is_empty()), - simple_tiles: { - let mut all = gauges.clone(); - all.extend(detail_groups.iter().flat_map(|g| g.gauges.iter().cloned())); - simple_tiles(&gauges, &all, &entities) - }, + health: plain::assess(&state, &all, !all.is_empty()), + simple_tiles: simple_tiles(&gauges, &all, &entities), gauges, detail_groups, entities, @@ -492,6 +520,58 @@ mod tests { } } + #[test] + fn state_changes_update_the_verdict_without_waiting_for_a_tick() { + let mut snapshot = TargetSnapshot::placeholder("t1", "host", ConnectionState::Online); + snapshot.health = plain::assess(&ConnectionState::Online, &[], true); + assert_eq!(snapshot.health.level, "ok"); + snapshot.set_state(ConnectionState::Failed { + message: "connection closed".into(), + recoverable: true, + }); + assert_eq!(snapshot.health.level, "offline"); + snapshot.set_state(ConnectionState::Idle); + assert_eq!(snapshot.health.headline, "Not connected"); + snapshot.set_state(ConnectionState::Connecting); + assert_eq!(snapshot.health.headline, "Connecting…"); + } + + #[test] + fn stopping_discards_queued_commands_before_a_restart() { + let core = ServerGlass::new(); + let id = core.add_target(config("127.0.0.1")); + core.start(id.clone()).unwrap(); + let target = core.target(&id).unwrap(); + let old_sender = target + .task + .lock() + .unwrap() + .as_ref() + .unwrap() + .commands + .clone(); + let (reply, answer) = tokio::sync::oneshot::channel(); + old_sender + .send(("must not run later".into(), reply)) + .unwrap(); + core.stop(id.clone()).unwrap(); + assert_eq!( + core.snapshot(id.clone()).unwrap().health.headline, + "Not connected" + ); + core.start(id.clone()).unwrap(); + core.runtime.block_on(async { + assert!( + tokio::time::timeout(std::time::Duration::from_secs(2), answer) + .await + .unwrap() + .is_err() + ); + }); + assert!(old_sender.is_closed()); + core.stop(id).unwrap(); + } + #[test] fn targets_get_distinct_ids_and_a_placeholder_snapshot() { let core = ServerGlass::new(); diff --git a/crates/sg-ffi/src/view.rs b/crates/sg-ffi/src/view.rs index 33590b2..ed787e7 100644 --- a/crates/sg-ffi/src/view.rs +++ b/crates/sg-ffi/src/view.rs @@ -231,6 +231,19 @@ pub struct TargetSnapshot { } impl TargetSnapshot { + /// State and verdict are one published value. Otherwise a disconnected host can keep saying + /// "Everything looks good" indefinitely, including on every platform's summary screen. + pub(crate) fn set_state(&mut self, state: ConnectionState) { + let mut all = self.gauges.clone(); + all.extend( + self.detail_groups + .iter() + .flat_map(|group| group.gauges.iter().cloned()), + ); + self.health = crate::plain::assess(&state, &all, !all.is_empty()); + self.state = state; + } + /// A snapshot for a target that has produced nothing yet. pub fn placeholder(target_id: &str, host: &str, state: ConnectionState) -> Self { let state_for_placeholder = state.clone(); diff --git a/crates/sg-sync/src/pairing.rs b/crates/sg-sync/src/pairing.rs index d8c38db..2c71475 100644 --- a/crates/sg-sync/src/pairing.rs +++ b/crates/sg-sync/src/pairing.rs @@ -27,8 +27,7 @@ pub const VERSION: u8 = 1; /// How long an offer is worth showing before it should be regenerated. /// -/// Not enforced by the protocol — a stale QR simply fails to connect once the listener is gone — -/// but the UI should expire the code so a screen left unlocked on a desk is not an open door. +/// Enforced by the listener as well as the UI so a forgotten pairing dialog cannot listen forever. pub const OFFER_TTL_SECS: u64 = 120; /// What the QR encodes. @@ -74,6 +73,9 @@ impl Offer { pub fn decode(text: &str) -> Result { let b64 = base64::engine::general_purpose::URL_SAFE_NO_PAD; let text = text.trim(); + if text.len() > 4096 { + return Err(SyncError::Malformed("pairing code is too large".into())); + } let rest = text .strip_prefix("SG") @@ -118,6 +120,9 @@ impl Offer { if addresses.is_empty() { return Err(SyncError::Malformed("no address".into())); } + if addresses.len() > 32 { + return Err(SyncError::Malformed("too many pairing addresses".into())); + } Ok(Offer { public_key: key, diff --git a/crates/sg-sync/src/transfer.rs b/crates/sg-sync/src/transfer.rs index 77a18c8..b494d3a 100644 --- a/crates/sg-sync/src/transfer.rs +++ b/crates/sg-sync/src/transfer.rs @@ -6,6 +6,7 @@ use tokio::io::{AsyncReadExt, AsyncWriteExt}; use tokio::net::{TcpListener, TcpStream}; +use tokio::time::{timeout, timeout_at, Duration, Instant}; use crate::pairing::{Handshake, Offer, Session}; use crate::SyncError; @@ -19,8 +20,10 @@ const MAX_TRANSFER: usize = 1 << 20; /// A device showing a QR and waiting for the other to connect. pub struct Listener { listener: TcpListener, + listener_v6: Option, handshake: Handshake, offer: Offer, + expires_at: Instant, } impl Listener { @@ -32,8 +35,8 @@ impl Listener { /// device is, and a device on WireGuard or Tailscale is often reachable at *only* the tunnel /// address. /// - /// The socket itself binds `0.0.0.0`, so it is listening on every interface regardless; the - /// list only decides what the other device is told to dial. + /// Listen on both address families when requested. Separate sockets avoid depending on the + /// operating system's default for IPv4-mapped IPv6 sockets. pub async fn bind(advertise_hosts: &[String]) -> Result { if advertise_hosts.is_empty() { return Err(SyncError::Transfer( @@ -47,24 +50,45 @@ impl Listener { .local_addr() .map_err(|e| SyncError::Transfer(e.to_string()))? .port(); + let listener_v6 = if advertise_hosts.iter().any(|host| host.contains(':')) { + TcpListener::bind("[::]:0").await.ok() + } else { + None + }; + let port_v6 = listener_v6 + .as_ref() + .and_then(|listener| listener.local_addr().ok()) + .map(|address| address.port()); - let addresses = advertise_hosts + let addresses: Vec<_> = advertise_hosts .iter() - .map(|host| { + .filter_map(|host| { // An IPv6 literal has to be bracketed before a port is appended, or `host:port` // is unparseable. A VPN handing out v6 is exactly where this bites. - if host.contains(':') && !host.starts_with('[') { - format!("[{host}]:{port}") + if host.contains(':') { + let port = port_v6?; + Some(if host.starts_with('[') { + format!("{host}:{port}") + } else { + format!("[{host}]:{port}") + }) } else { - format!("{host}:{port}") + Some(format!("{host}:{port}")) } }) .collect(); + if addresses.is_empty() { + return Err(SyncError::Transfer( + "could not listen on any of this device's network addresses".into(), + )); + } let (handshake, offer) = Handshake::offering(addresses); Ok(Listener { listener, + listener_v6, handshake, offer, + expires_at: Instant::now() + Duration::from_secs(crate::pairing::OFFER_TTL_SECS), }) } @@ -77,11 +101,28 @@ impl Listener { /// Returns before anything is transferred: the caller must show the verification code and get /// the user's confirmation first. That ordering is the point of the code. pub async fn accept(self) -> Result<(Session, TcpStream), SyncError> { - let (mut stream, _) = self - .listener - .accept() + let expired = || SyncError::Transfer("The pairing code expired. Create a new one.".into()); + // timeout_at polls the future first. Reject a stale offer even if the socket and + // handshake bytes are queued and the exchange could complete without yielding. + if Instant::now() >= self.expires_at { + return Err(expired()); + } + timeout_at(self.expires_at, self.accept_inner()) .await - .map_err(|e| SyncError::Transfer(format!("no device connected: {e}")))?; + .map_err(|_| expired())? + } + + async fn accept_inner(self) -> Result<(Session, TcpStream), SyncError> { + let accepted = if let Some(listener_v6) = &self.listener_v6 { + tokio::select! { + accepted = self.listener.accept() => accepted, + accepted = listener_v6.accept() => accepted, + } + } else { + self.listener.accept().await + }; + let (mut stream, _) = + accepted.map_err(|e| SyncError::Transfer(format!("no device connected: {e}")))?; let mut theirs = [0u8; 32]; stream @@ -116,40 +157,41 @@ const PER_ADDRESS_TIMEOUT: std::time::Duration = std::time::Duration::from_secs( /// reach a human first. pub async fn send_transfer(offer: &Offer) -> Result<(Session, TcpStream), SyncError> { let mut last: Option = None; - let mut stream = None; - for address in &offer.addresses { - match tokio::time::timeout(PER_ADDRESS_TIMEOUT, TcpStream::connect(address)).await { - Ok(Ok(connected)) => { - stream = Some(connected); - break; + // A TCP accept is not a successful handshake. Bound the entire exchange and keep trying + // candidate addresses if a VPN endpoint or unrelated listener answers but never speaks. + let attempt = async { + let mut stream = TcpStream::connect(address) + .await + .map_err(|e| SyncError::Transfer(e.to_string()))?; + let handshake = Handshake::accepting(offer); + stream + .write_all(&handshake.public_key()) + .await + .map_err(|e| SyncError::Transfer(e.to_string()))?; + let mut theirs = [0u8; 32]; + stream + .read_exact(&mut theirs) + .await + .map_err(|e| SyncError::Transfer(e.to_string()))?; + if theirs != offer.public_key { + return Err(SyncError::Handshake( + "The device's key does not match the pairing code.".into(), + )); } - Ok(Err(e)) => last = Some(format!("{address}: {e}")), + let session = handshake.complete(theirs, false)?; + Ok((session, stream)) + }; + match timeout(PER_ADDRESS_TIMEOUT, attempt).await { + Ok(Ok(connected)) => return Ok(connected), + Ok(Err(error)) => last = Some(format!("{address}: {error}")), Err(_) => last = Some(format!("{address}: timed out")), } } - - let mut stream = stream.ok_or_else(|| { - SyncError::Transfer(format!( - "could not reach the other device. Check both are on the same network or VPN{}", - last.map(|e| format!(" ({e})")).unwrap_or_default() - )) - })?; - - let handshake = Handshake::accepting(offer); - stream - .write_all(&handshake.public_key()) - .await - .map_err(|e| SyncError::Transfer(format!("handshake failed: {e}")))?; - - let mut theirs = [0u8; 32]; - stream - .read_exact(&mut theirs) - .await - .map_err(|e| SyncError::Transfer(format!("handshake failed: {e}")))?; - - let session = handshake.complete(theirs, false)?; - Ok((session, stream)) + Err(SyncError::Transfer(format!( + "could not reach the other device. Check both are on the same network or VPN{}", + last.map(|error| format!(" ({error})")).unwrap_or_default() + ))) } /// Send the sealed payload. Call only after the user has confirmed the codes match. @@ -158,6 +200,25 @@ pub async fn write_payload( stream: &mut TcpStream, plaintext: &[u8], ) -> Result<(), SyncError> { + timeout( + Duration::from_secs(30), + write_payload_inner(session, stream, plaintext), + ) + .await + .map_err(|_| SyncError::Transfer("The transfer timed out. Pair the devices again.".into()))? +} + +async fn write_payload_inner( + session: &Session, + stream: &mut TcpStream, + plaintext: &[u8], +) -> Result<(), SyncError> { + // 12 bytes of nonce and 16 bytes of authentication tag count toward the wire limit. + if plaintext.len() > MAX_TRANSFER - 28 { + return Err(SyncError::Transfer( + "the inventory is too large to send".into(), + )); + } let sealed = session.seal(plaintext)?; let len = u32::try_from(sealed.len()) .map_err(|_| SyncError::Transfer("the inventory is too large to send".into()))?; @@ -181,6 +242,18 @@ pub async fn write_payload( pub async fn accept_transfer( session: &Session, stream: &mut TcpStream, +) -> Result, SyncError> { + timeout( + Duration::from_secs(30), + accept_transfer_inner(session, stream), + ) + .await + .map_err(|_| SyncError::Transfer("The transfer timed out. Pair the devices again.".into()))? +} + +async fn accept_transfer_inner( + session: &Session, + stream: &mut TcpStream, ) -> Result, SyncError> { let mut len = [0u8; 4]; stream diff --git a/crates/sg-sync/tests/pairing_over_tcp.rs b/crates/sg-sync/tests/pairing_over_tcp.rs index 06d16eb..44c366b 100644 --- a/crates/sg-sync/tests/pairing_over_tcp.rs +++ b/crates/sg-sync/tests/pairing_over_tcp.rs @@ -24,6 +24,71 @@ fn inventory() -> Payload { } } +#[tokio::test] +async fn pairing_listens_on_every_advertised_address_family() { + for host in ["127.0.0.1", "::1"] { + let listener = Listener::bind(&[host.into()]).await.expect("bind loopback"); + let offer = listener.offer().clone(); + let accepting = tokio::spawn(listener.accept()); + let (sender, _) = send_transfer(&offer) + .await + .expect("reach advertised address"); + let (receiver, _) = accepting.await.unwrap().unwrap(); + assert_eq!(sender.verification_code(), receiver.verification_code()); + } +} + +#[tokio::test] +async fn a_listener_that_never_completes_the_handshake_does_not_block_later_addresses() { + let silent = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let silent_address = silent.local_addr().unwrap().to_string(); + let listener = Listener::bind(&["127.0.0.1".into()]).await.unwrap(); + let mut offer = listener.offer().clone(); + offer.addresses.insert(0, silent_address); + let accepting = tokio::spawn(listener.accept()); + let silence = tokio::spawn(async move { + let (_stream, _) = silent.accept().await.unwrap(); + std::future::pending::<()>().await; + }); + let (session, _) = tokio::time::timeout(Duration::from_secs(8), send_transfer(&offer)) + .await + .expect("a silent address blocked the valid one") + .unwrap(); + let (receiver, _) = accepting.await.unwrap().unwrap(); + assert_eq!(session.verification_code(), receiver.verification_code()); + silence.abort(); +} + +#[tokio::test] +async fn a_device_with_a_different_key_than_the_qr_is_rejected() { + use tokio::io::AsyncWriteExt; + let impostor = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let (_, offer) = + sg_sync::pairing::Handshake::offering(vec![impostor.local_addr().unwrap().to_string()]); + let answering = tokio::spawn(async move { + let (mut stream, _) = impostor.accept().await.unwrap(); + stream.write_all(&[1; 32]).await.unwrap(); + }); + let error = send_transfer(&offer) + .await + .err() + .expect("mismatched QR key accepted"); + assert!(error.to_string().contains("does not match")); + answering.await.unwrap(); +} + +#[tokio::test] +async fn an_oversize_inventory_is_rejected_before_writing() { + let listener = Listener::bind(&["127.0.0.1".into()]).await.unwrap(); + let offer = listener.offer().clone(); + let accepting = tokio::spawn(listener.accept()); + let (session, mut stream) = send_transfer(&offer).await.unwrap(); + let _receiver = accepting.await.unwrap().unwrap(); + assert!(write_payload(&session, &mut stream, &vec![0; 1 << 20]) + .await + .is_err()); +} + #[tokio::test(flavor = "multi_thread")] async fn an_inventory_crosses_between_two_devices() { // The device being set up shows the QR. diff --git a/crates/sg-transport/src/error.rs b/crates/sg-transport/src/error.rs index 726d8ba..ae4e8bc 100644 --- a/crates/sg-transport/src/error.rs +++ b/crates/sg-transport/src/error.rs @@ -25,6 +25,12 @@ pub enum TransportError { fingerprint: String, }, + #[error("could not read or save this server's trusted identity: {detail}")] + HostKeyStorage { detail: String }, + + #[error("the server's response exceeded the {limit}-byte output limit")] + OutputTooLarge { limit: usize }, + #[error("authentication failed for {user}@{host}")] AuthFailed { user: String, host: String }, @@ -74,6 +80,8 @@ impl TransportError { TransportError::AuthFailed { .. } | TransportError::UnknownHostKey { .. } | TransportError::HostKeyMismatch { .. } + | TransportError::HostKeyStorage { .. } + | TransportError::OutputTooLarge { .. } | TransportError::KeyFile { .. } | TransportError::KeyText { .. } | TransportError::NoAgent(_) diff --git a/crates/sg-transport/src/frame.rs b/crates/sg-transport/src/frame.rs index b85cdae..aa58b8c 100644 --- a/crates/sg-transport/src/frame.rs +++ b/crates/sg-transport/src/frame.rs @@ -94,7 +94,7 @@ impl Framing { // Braces group the command so the redirect applies to all of it; `__sg` holds the // status because the printf that follows would otherwise clobber `$?`. script.push_str(&format!( - "printf {}\n{{ {}; }} 2>/dev/null\n__sg=$?\nprintf {} \"$__sg\"\n", + "printf {}\n{{ {}; }} /dev/null\n__sg=$?\nprintf {} \"$__sg\"\n", shell_quote(&self.begin(&id)), command, shell_quote(&format!("{}%s\n", self.end_prefix(&id))), diff --git a/crates/sg-transport/src/session.rs b/crates/sg-transport/src/session.rs index 64f9ec6..11c3c93 100644 --- a/crates/sg-transport/src/session.rs +++ b/crates/sg-transport/src/session.rs @@ -13,7 +13,7 @@ use russh::keys::agent::client::AgentClient; use russh::keys::{decode_secret_key, load_secret_key, PrivateKeyWithHashAlg}; use russh::{ChannelMsg, Disconnect}; use sg_model::{Request, Responses}; -use tokio::time::timeout; +use tokio::time::{timeout, timeout_at, Instant}; use crate::auth::{Auth, ConnectionSpec, HostKeyPolicy}; use crate::error::{Result, TransportError}; @@ -24,9 +24,8 @@ use crate::frame::Framing; #[derive(Clone, Debug, PartialEq, Eq)] enum HostKeyVerdict { Accepted, - /// Trusted as asked, but the key could not be written down — so the next connection has - /// nothing to compare against and would trust a different key just as readily. - AcceptedUnrecorded { + /// Verification cannot be completed safely if the pin store cannot be read or written. + StorageFailed { detail: String, }, Unknown { @@ -50,8 +49,11 @@ impl client::Handler for ClientHandler { async fn check_server_key( &mut self, - server_public_key: &russh::keys::ssh_key::PublicKey, + server_key: &russh::keys::PublicKeyOrCertificate, ) -> std::result::Result { + // Pin the leaf key, including when it is carried by a certificate. We do not implicitly + // trust a certificate authority or change the application's existing pinning policy. + let server_public_key = &server_key.public_key(); let fingerprint = server_public_key .fingerprint(russh::keys::HashAlg::Sha256) .to_string(); @@ -61,33 +63,49 @@ impl client::Handler for ClientHandler { return Ok(true); } - let known = match &self.known_hosts { - Some(path) => { - russh::keys::check_known_hosts_path(&self.host, self.port, server_public_key, path) - } - None => russh::keys::check_known_hosts(&self.host, self.port, server_public_key), + let path = self + .known_hosts + .clone() + .or_else(|| std::env::home_dir().map(|home| home.join(".ssh/known_hosts"))); + let Some(path) = path else { + self.record(HostKeyVerdict::StorageFailed { + detail: "No home directory for known_hosts.".into(), + }); + return Ok(false); }; + // russh treats *any* open failure as an unknown host. Only a missing file means first + // use; a permission failure must not silently downgrade an existing pin. Serialise the + // read/learn pair so simultaneous connections cannot race to trust two different keys. + static PIN_STORE: Mutex<()> = Mutex::new(()); + let _guard = PIN_STORE + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + if let Err(error) = std::fs::File::open(&path) { + if error.kind() != std::io::ErrorKind::NotFound { + self.record(HostKeyVerdict::StorageFailed { + detail: error.to_string(), + }); + return Ok(false); + } + } + let known = + russh::keys::check_known_hosts_path(&self.host, self.port, server_public_key, &path); let verdict = match known { Ok(true) => HostKeyVerdict::Accepted, // A key recorded for this host that does not match the one presented. Never // acceptable under any policy — this is the case host key checking exists for. Err(russh::keys::Error::KeyChanged { .. }) => HostKeyVerdict::Changed { fingerprint }, - Ok(false) | Err(_) => { + Err(error) => HostKeyVerdict::StorageFailed { + detail: error.to_string(), + }, + Ok(false) => { if self.policy == HostKeyPolicy::AcceptNew { - if let Err(error) = learn( - &self.host, - self.port, - server_public_key, - self.known_hosts.as_deref(), - ) { - // Accept anyway — the user asked to trust this host — but say that the - // key was not recorded, because "trusted on first use" with nothing - // written down means *every* connection is a first use, and the next one - // would accept a different key just as readily. - self.record(HostKeyVerdict::AcceptedUnrecorded { + if let Err(error) = learn(&self.host, self.port, server_public_key, &path) { + // Do not send credentials until the promised pin is durable. + self.record(HostKeyVerdict::StorageFailed { detail: error.to_string(), }); - return Ok(true); + return Ok(false); } HostKeyVerdict::Accepted } else { @@ -102,48 +120,31 @@ impl client::Handler for ClientHandler { } } -/// Record a host key, creating the directory it lives in. -/// -/// `learn_known_hosts` writes to `~/.ssh/known_hosts` and does not create `~/.ssh`. On a desktop -/// that directory is always there; in an app sandbox it is not, so on Android and iOS the write -/// failed every time — and the failure was discarded. The apps offered "remember its identity the -/// first time you connect", recorded nothing, and then accepted whatever key was presented on -/// every subsequent connection, which is the exact attack host-key checking exists to stop. +/// Record a host key, creating its directory without changing permissions on an existing one. fn learn( host: &str, port: u16, key: &russh::keys::PublicKey, - known_hosts: Option<&std::path::Path>, + known_hosts: &std::path::Path, ) -> std::result::Result<(), std::io::Error> { - let directory = match known_hosts { - Some(path) => path.parent().map(std::path::Path::to_path_buf), - // Nothing configured: the desktop case, where `~/.ssh` is what russh will use. Windows - // has no `HOME` — it has `USERPROFILE` — and looking only at the former would have skipped - // creating the directory there, reintroducing on Windows exactly the silent failure this - // function exists to fix. - None => std::env::var_os("HOME") - .or_else(|| std::env::var_os("USERPROFILE")) - .map(|home| std::path::Path::new(&home).join(".ssh")), - }; - - if let Some(directory) = directory { - // `learn_known_hosts` writes the file but will not create the directory holding it, which - // is why this failed in an app sandbox where nothing had made one. - std::fs::create_dir_all(&directory)?; - // 0700: a known_hosts file is not secret, but the directory it shares with private keys - // should not be readable by anything else on a multi-user box. + if let Some(directory) = known_hosts + .parent() + .filter(|path| !path.as_os_str().is_empty()) + { + let existed = directory.exists(); + std::fs::create_dir_all(directory)?; + // A caller may keep pins in an existing shared directory. chmod on that directory would + // change permissions for unrelated files (even /tmp), so only secure directories we made. #[cfg(unix)] - { + if !existed { use std::os::unix::fs::PermissionsExt; - let _ = std::fs::set_permissions(&directory, std::fs::Permissions::from_mode(0o700)); + std::fs::set_permissions(directory, std::fs::Permissions::from_mode(0o700))?; } + #[cfg(not(unix))] + let _ = existed; } - - match known_hosts { - Some(path) => russh::keys::known_hosts::learn_known_hosts_path(host, port, key, path), - None => russh::keys::known_hosts::learn_known_hosts(host, port, key), - } - .map_err(|e| std::io::Error::other(e.to_string())) + russh::keys::known_hosts::learn_known_hosts_path(host, port, key, known_hosts) + .map_err(|error| std::io::Error::other(error.to_string())) } impl ClientHandler { @@ -212,21 +213,31 @@ impl SshSession { fingerprint, } } + Some(HostKeyVerdict::StorageFailed { detail }) => { + TransportError::HostKeyStorage { detail } + } _ => TransportError::Ssh(err), }); } }; - authenticate(&mut handle, &spec).await?; - - let channel = handle.channel_open_session().await?; - // No PTY, deliberately. A PTY would echo the script back into the output stream and - // translate LF to CRLF, corrupting every payload we parse. - // - // `/bin/sh` rather than the login shell: the user's shell might be fish or csh, whose - // syntax our batch script is not written in. Executing sh explicitly makes the protocol - // independent of whatever the account happens to be configured with. - channel.exec(true, "/bin/sh").await?; + let channel = timeout(Duration::from_millis(spec.connect_timeout_ms), async { + authenticate(&mut handle, &spec).await?; + let channel = handle.channel_open_session().await?; + // No PTY, deliberately. A PTY would echo the script back into the output stream and + // translate LF to CRLF, corrupting every payload we parse. + // + // `/bin/sh` rather than the login shell: the user's shell might be fish or csh, whose + // syntax our batch script is not written in. Executing sh explicitly makes the protocol + // independent of whatever the account happens to be configured with. + channel.exec(true, "/bin/sh").await?; + Ok::<_, TransportError>(channel) + }) + .await + .map_err(|_| TransportError::Timeout { + what: "authentication and shell startup", + ms: spec.connect_timeout_ms, + })??; let mut session = SshSession { spec, @@ -255,14 +266,21 @@ impl SshSession { } self.round_trips += 1; - self.raw_write(&script).await?; + // One absolute deadline, including the write. Resetting a relative timeout for every + // packet lets a streaming command run forever and grow the buffer without bound. + let deadline = Instant::now() + Duration::from_millis(self.spec.batch_timeout_ms); + timeout_at(deadline, self.raw_write(&script)) + .await + .map_err(|_| TransportError::Timeout { + what: "collection batch", + ms: self.spec.batch_timeout_ms, + })??; let terminator = self.framing.terminator(); - let deadline = Duration::from_millis(self.spec.batch_timeout_ms); let mut buf: Vec = Vec::with_capacity(16 * 1024); loop { - match timeout(deadline, self.channel.wait()).await { + match timeout_at(deadline, self.channel.wait()).await { Err(_) => { return Err(TransportError::Timeout { what: "collection batch", @@ -272,6 +290,12 @@ impl SshSession { Ok(None) => return Err(TransportError::Closed), Ok(Some(msg)) => match msg { ChannelMsg::Data { ref data } => { + const MAX_BATCH_BYTES: usize = 16 * 1024 * 1024; + if data.len() > MAX_BATCH_BYTES.saturating_sub(buf.len()) { + return Err(TransportError::OutputTooLarge { + limit: MAX_BATCH_BYTES, + }); + } buf.extend_from_slice(data); // Search only the tail: the terminator cannot straddle more than its own // length, and rescanning a megabyte of process table every chunk is diff --git a/crates/sg-transport/tests/live.rs b/crates/sg-transport/tests/live.rs index d377c15..f8352ef 100644 --- a/crates/sg-transport/tests/live.rs +++ b/crates/sg-transport/tests/live.rs @@ -79,6 +79,76 @@ macro_rules! session_or_skip { }; } +#[tokio::test] +async fn commands_cannot_consume_the_framing_script_from_stdin() { + for port in [DEBIAN_PORT, ALPINE_PORT] { + let mut session = session_or_skip!(port); + let cat = Request::exec(["cat"]); + let next = Request::exec(["printf", "still framed"]); + let responses = session + .batch(&[cat.clone(), next.clone()]) + .await + .expect("batch"); + assert_eq!(responses.text(&cat), Some("")); + assert_eq!(responses.text(&next), Some("still framed")); + } +} + +#[tokio::test] +async fn streaming_output_does_not_extend_the_batch_deadline() { + if !fixture_up(DEBIAN_PORT).await { + return; + } + let mut config = spec(DEBIAN_PORT); + config.batch_timeout_ms = 200; + let mut session = SshSession::connect(config).await.expect("connect"); + let request = Request::exec(["sh", "-c", "while :; do printf x; sleep 0.05; done"]); + let started = std::time::Instant::now(); + let result = tokio::time::timeout(Duration::from_secs(3), session.batch(&[request])) + .await + .expect("absolute deadline"); + assert!(matches!( + result, + Err(sg_transport::TransportError::Timeout { .. }) + )); + assert!(started.elapsed() < Duration::from_secs(2)); +} + +#[tokio::test] +async fn excessive_output_is_bounded() { + if !fixture_up(DEBIAN_PORT).await { + return; + } + let mut session = session_or_skip!(DEBIAN_PORT); + let result = session + .batch(&[Request::exec(["head", "-c", "18000000", "/dev/zero"])]) + .await; + assert!(matches!( + result, + Err(sg_transport::TransportError::OutputTooLarge { .. }) + )); +} + +#[tokio::test] +async fn an_unwritable_pin_is_a_visible_nonrecoverable_failure() { + if !fixture_up(DEBIAN_PORT).await { + return; + } + // A directory is readable but cannot be used as a pin file, on Unix and Windows alike. + let config = spec(DEBIAN_PORT) + .known_hosts(std::env::temp_dir()) + .host_key_policy(HostKeyPolicy::AcceptNew); + let error = SshSession::connect(config) + .await + .err() + .expect("must not connect without a pin"); + assert!(matches!( + error, + sg_transport::TransportError::HostKeyStorage { .. } + )); + assert!(!error.is_transient()); +} + #[tokio::test] async fn reads_proc_files_over_one_channel() { let mut session = session_or_skip!(DEBIAN_PORT); @@ -347,12 +417,11 @@ async fn trusting_a_host_on_first_use_actually_records_its_key() { if !fixture_up(DEBIAN_PORT).await { return; } - // A private HOME, so this neither reads nor writes the developer's own known_hosts. + // A dedicated pin path. Changing HOME here races the other tests and library threads. let home = std::env::temp_dir().join(format!("sg-known-hosts-{}", std::process::id())); let _ = std::fs::remove_dir_all(&home); std::fs::create_dir_all(&home).expect("temp home"); - // SAFETY: single-threaded test process; russh reads HOME when it resolves known_hosts. - unsafe { std::env::set_var("HOME", &home) }; + let recorded = home.join(".ssh").join("known_hosts"); let spec = ConnectionSpec::new("127.0.0.1", "root") .port(DEBIAN_PORT) @@ -360,12 +429,12 @@ async fn trusting_a_host_on_first_use_actually_records_its_key() { path: fixture_key(), passphrase: None, }) - .host_key_policy(HostKeyPolicy::AcceptNew); + .host_key_policy(HostKeyPolicy::AcceptNew) + .known_hosts(&recorded); let session = SshSession::connect(spec).await.expect("connect"); drop(session); - let recorded = home.join(".ssh").join("known_hosts"); assert!( recorded.exists(), "the host key was not written to {}", diff --git a/docs/RELIABILITY_AUDIT.md b/docs/RELIABILITY_AUDIT.md new file mode 100644 index 0000000..5ecf8e8 --- /dev/null +++ b/docs/RELIABILITY_AUDIT.md @@ -0,0 +1,71 @@ +# Reliability and UI audit — 28 September 2026 + +Work is on `codex/reliability-and-ui`. All eight pull-request heads were fetched. PRs 1–4 were +already merged; the four open dependency PRs (5–8) were integrated locally. This audit does not +merge or close those PRs on GitHub. + +## Reported macOS crash + +The supplied report traps in Swift's `ServerGlass.addTarget(config:)` while restoring saved hosts. +The installed executable loads `libsg_ffi.dylib` from an absolute path in a development checkout. +SwiftPM's `-lsg_ffi` selected that dylib even though the build intended to embed the static core. +A rebuilt library can therefore change the native record layout underneath an installed app. + +The package now links the archive explicitly. Packaging rejects any remaining `libsg_ffi` dynamic +dependency, verifies its signature, and takes the bundle version from the workspace. The rebuilt +app launches and collects live readings. A Swift regression test sends a complete saved-host +configuration through the native bridge. The external-library dependency is confirmed; the exact +historical library/bindings mismatch cannot be reconstructed from the crash report alone. + +## Confirmed defects addressed + +| Area | Failure | Change | +| --- | --- | --- | +| Dependency integration | The russh update no longer implemented the required host-key callback. | Support the new key/certificate argument while preserving leaf-key pinning. | +| Android build | AGP 9 conflicts with the old Kotlin Android plugin; current UI dependencies need a newer compile SDK. | Use AGP's built-in Kotlin support, register generated Kotlin sources correctly, and compile with SDK 37. Minimum and target SDK remain unchanged. | +| Linux build | Direct GLib 0.22 types conflict with GTK 0.9 and exceed the pinned Rust toolchain's minimum. | Align GLib with GTK's 0.20 dependency and regenerate the separate lockfile. | +| SSH trust | A failed pin write still permitted authentication; unreadable pins could look like first use. | Fail before authentication, report storage errors, and serialize pin checks/writes within the process. Preserve permissions on existing directories. | +| SSH framing | A command reading stdin could consume the rest of the batch script. | Give each command `/dev/null` as stdin. Test on Debian and Alpine. | +| SSH bounds | Streaming output repeatedly renewed its timeout and could grow memory indefinitely. | Use an absolute deadline, a 16 MiB output bound, and bounded authentication/channel startup. | +| Poll lifecycle | Stopped tasks could publish old state; queued commands could survive a restart. | Give each run its own command queue and generation, reject disconnected commands, and discard abandoned requests. | +| Reconnection | A command transport failure broke only the inner command loop. | Leave the refresh loop and rebuild the failed session. | +| Health | A healthy verdict could survive disconnection; detail gauges were omitted from assessment. | Recompute health on state transitions and assess all host gauges. | +| Metrics | Counter state and vanished metric histories accumulated as processes/devices changed. | Prune against each complete tick; rebaseline reused IDs and ignore non-finite samples. | +| Credential storage | Apple deleted a credential before knowing its replacement could be saved. Android silently fell back to plaintext storage. | Update Keychain items in place; require encrypted Android storage and migrate legacy values only after a successful encrypted write. | +| Inventory | Unreadable saved inventories could become an empty list and then be overwritten. | Report unreadable data and prevent normal save flows from replacing it on all four front-ends. | +| Windows lifetime | Disposing the native core could race with a background native call. | Use SafeHandle with a lease for every call, including blocking commands and pairing. | +| Windows polling | A background thread enumerated an ObservableCollection owned by the UI. | Copy hosts on the dispatcher, skip obsolete updates, and report unexpected polling failures. | +| Pairing | A TCP listener that never answered could block later addresses; transfers had no deadline. | Bound the full handshake and payload, verify the QR's public key, enforce offer expiry and payload limits. | +| IPv6 pairing | Offers advertised IPv6 addresses while the listener bound only IPv4. | Listen on both requested address families and advertise only sockets that were opened. | +| Apple interaction | Deleting several rows could use shifted indices; compact fleet cards did not navigate. | Capture IDs before deletion and use NavigationLink for phone fleet cards. | +| Presentation | Failed connections could show an endless spinner or stale fleet metrics; some proportions were drawn without a maximum. | Make empty states reflect connection health, hide stale fleet readings, and draw capacity widgets only for real fractions. | +| UI consistency | Process severity was calculated again in some front-ends; supporting text was faint. | Use the core's severity and improve text contrast across platforms, plus native text sizes for Apple host rows. | + +The fixes retain read-only collection, one network round trip per refresh, in-memory samples, +device-local credentials, and shared Rust formatting/severity decisions. + +## Verification + +| Surface | Evidence | +| --- | --- | +| Rust workspace | `SG_REQUIRE_FIXTURES=1 ./scripts/check.sh`: format, Clippy, build, 285 tests including live Debian/Alpine tests, Linux lockfile resolution and version checks. The final pairing expiry adjustment also passed targeted tests and workspace Clippy. | +| macOS | Built and signed the bundle; inspected its linked libraries; 10 Swift tests; launched against the Debian fixture and inspected live technical/summary views. | +| iOS | Built for the arm64 simulator; launched on iPhone 17 Pro; verified live readings, back navigation and Add Host validation (port 0 disabled, valid port enabled). | +| Android | Built the arm64 debug APK; all 9 JVM tests passed; installed and launched on the foldable emulator with no recorded crash. This was a fixture launch, not a complete interactive Android UI test. | +| Linux | Built in a Rust 1.89 Debian container with GTK/libadwaita; format and Clippy passed with warnings denied; 24 unit tests and 4 live engine tests passed. | +| Windows bridge | Built the .NET projects and passed 24 tests against the real native library on macOS, including concurrent disposal. DPAPI tests were excluded because they require Windows. | +| CI configuration | Added the macOS packaging/bridge job and required live fixtures for Linux jobs; YAML parses locally. The updated workflow has not yet run on GitHub. | + +## Limits and release follow-up + +- WinUI build/launch, Windows installation and DPAPI must still be verified on Windows. +- Linux desktop rendering and complete Android interaction need native UI testing. An iPad and + physical iOS device were not exercised. +- This pass is not proof that the repository contains no remaining bugs. It does not establish + accessibility conformance, long-duration stability or an exhaustive security audit. +- Changes and build artifacts are local. The existing app in `/Applications` was not replaced, + and no release was published. + +Useful local outputs are `target/ServerGlass.app`, the iOS simulator app under +`target/ios/DerivedData/Build/Products/Debug-iphonesimulator`, and +`apps/android/app/build/outputs/apk/debug/app-debug.apk`. diff --git a/scripts/build-android.sh b/scripts/build-android.sh index c43b7e7..48ae0df 100755 --- a/scripts/build-android.sh +++ b/scripts/build-android.sh @@ -7,7 +7,7 @@ # Prerequisites, all installed into user space with no sudo: # brew install openjdk@21 gradle # brew install --cask android-commandlinetools -# sdkmanager platform-tools "platforms;android-35" "build-tools;36.1.0" \ +# sdkmanager platform-tools "platforms;android-37" "build-tools;36.1.0" \ # "ndk;27.3.13750724" emulator "system-images;android-36;google_apis;arm64-v8a" # cargo install cargo-ndk && rustup target add aarch64-linux-android set -euo pipefail diff --git a/scripts/build-ios.sh b/scripts/build-ios.sh index e16096f..369ab52 100755 --- a/scripts/build-ios.sh +++ b/scripts/build-ios.sh @@ -8,6 +8,10 @@ # and the only change is the SDK and CODE_SIGNING settings. set -euo pipefail +# Xcode rejects a macOS deployment floor inherited by an iOS build. Scope each Rust build's +# floor below so its crypto C/assembly objects match the platform it will actually run on. +unset MACOSX_DEPLOYMENT_TARGET IPHONEOS_DEPLOYMENT_TARGET + cd "$(dirname "$0")/.." RUN=${1:-} GENERATED=apps/shared/ServerGlassFFI/generated @@ -15,7 +19,7 @@ SIM_TARGET=aarch64-apple-ios-sim DEVICE=${SG_SIM_DEVICE:-iPhone 17 Pro} echo "==> building sg-ffi for $SIM_TARGET" -cargo build -p sg-ffi --target "$SIM_TARGET" +IPHONEOS_DEPLOYMENT_TARGET=18.0 cargo build -p sg-ffi --target "$SIM_TARGET" # Xcode resolves -lsg_ffi from one directory, so the slice for the SDK being built goes there. mkdir -p target/ios @@ -24,7 +28,7 @@ cp "target/$SIM_TARGET/debug/libsg_ffi.a" target/ios/libsg_ffi.a echo "==> generating Swift bindings" # Generated from the host-architecture dylib: the metadata uniffi-bindgen reads is # architecture-independent, and a simulator .a has no dylib to introspect. -cargo build -p sg-ffi +MACOSX_DEPLOYMENT_TARGET=14.0 cargo build -p sg-ffi # `--bin` is not optional: the crate also ships a C# generator for the Windows app, so # an unqualified `cargo run` is ambiguous and fails. cargo run -q -p sg-bindgen --bin uniffi-bindgen -- generate \ diff --git a/scripts/build-macos.sh b/scripts/build-macos.sh index 23d459e..c63672c 100755 --- a/scripts/build-macos.sh +++ b/scripts/build-macos.sh @@ -8,8 +8,16 @@ # the compiled library's metadata, not the source. set -euo pipefail +# Applies to Rust's C/assembly dependencies too. Otherwise a newer Xcode quietly compiles the +# bundled crypto library for the build machine's OS, above the app's advertised minimum. +export MACOSX_DEPLOYMENT_TARGET=14.0 + cd "$(dirname "$0")/.." PROFILE="${1:-debug}" +case "$PROFILE" in + debug|release) ;; + *) echo "usage: $0 [debug|release]" >&2; exit 1 ;; +esac # The path `apps/Package.swift` actually consumes. It used to point under `apps/macos/`, which # nothing reads: the macOS build then compiled the UI against whatever bindings were last written # by `build-ios.sh`, so a change to the FFI surface reached iOS and silently did not reach macOS. @@ -52,6 +60,13 @@ rm -rf "$APP" mkdir -p "$APP/Contents/MacOS" "$APP/Contents/Resources" cp "apps/.build/$PROFILE/ServerGlass" "$APP/Contents/MacOS/ServerGlass" +# An installed bundle must never load a core from a development checkout. That can trap in +# addTarget after the checkout's dylib is rebuilt with a different UniFFI record layout. +if otool -L "$APP/Contents/MacOS/ServerGlass" | grep 'libsg_ffi' >/dev/null; then + echo "error: ServerGlass still dynamically links the Rust core" >&2 + exit 1 +fi + # The icon is generated from source (scripts/make-icons.swift) rather than committed as binaries. echo "==> icon" swift scripts/make-icons.swift >/dev/null @@ -76,4 +91,10 @@ cat > "$APP/Contents/Info.plist" <<'PLIST' PLIST +VERSION=$(sed -n 's/^version = "\([^"]*\)".*/\1/p' Cargo.toml | head -1) +/usr/libexec/PlistBuddy -c "Set :CFBundleShortVersionString $VERSION" "$APP/Contents/Info.plist" + +codesign --force --sign - "$APP" +codesign --verify --strict "$APP" + echo "built: $APP" diff --git a/scripts/release.sh b/scripts/release.sh index 8b4cc24..b49792a 100755 --- a/scripts/release.sh +++ b/scripts/release.sh @@ -50,7 +50,8 @@ build_macos() { # Ad-hoc signature. It does not satisfy Gatekeeper — the first launch still needs # right-click > Open, or `xattr -dr com.apple.quarantine` — but it does stop macOS treating # the binary as damaged, which is what an entirely unsigned bundle looks like. - codesign --force --deep --sign - "$staging/ServerGlass.app" 2>/dev/null || true + codesign --force --sign - "$staging/ServerGlass.app" + codesign --verify --strict "$staging/ServerGlass.app" rm -f "$dmg" hdiutil create -volname "ServerGlass" -srcfolder "$staging" -ov -format UDZO "$dmg" >/dev/null From fc007f07816605ef9453099130b0d5d2bebe2e87 Mon Sep 17 00:00:00 2001 From: lazarevtill Date: Mon, 28 Sep 2026 12:16:05 +0200 Subject: [PATCH 6/8] Clarify audit delivery status for pull request --- docs/RELIABILITY_AUDIT.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/RELIABILITY_AUDIT.md b/docs/RELIABILITY_AUDIT.md index 5ecf8e8..05bcf38 100644 --- a/docs/RELIABILITY_AUDIT.md +++ b/docs/RELIABILITY_AUDIT.md @@ -63,8 +63,8 @@ device-local credentials, and shared Rust formatting/severity decisions. physical iOS device were not exercised. - This pass is not proof that the repository contains no remaining bugs. It does not establish accessibility conformance, long-duration stability or an exhaustive security audit. -- Changes and build artifacts are local. The existing app in `/Applications` was not replaced, - and no release was published. +- Build artifacts remain local. The existing app in `/Applications` was not replaced, + and no release was published. Review and CI status are tracked in the branch's pull request. Useful local outputs are `target/ServerGlass.app`, the iOS simulator app under `target/ios/DerivedData/Build/Products/Debug-iphonesimulator`, and From 691fbc97285c96298e141e4d0503ebe24b133c71 Mon Sep 17 00:00:00 2001 From: lazarevtill Date: Mon, 28 Sep 2026 12:21:07 +0200 Subject: [PATCH 7/8] Select a Swift 6 toolchain for macOS CI --- .github/workflows/ci.yml | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3a70712..ca84dda 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -44,9 +44,16 @@ jobs: macos-app: name: macOS app and native bridge - runs-on: macos-14 + # The macOS 14 runner defaults to Swift 5.10; the package requires Swift 6. + runs-on: macos-15 + env: + DEVELOPER_DIR: /Applications/Xcode_16.4.app/Contents/Developer steps: - uses: actions/checkout@v7 + - name: Show the Apple toolchain + run: | + xcodebuild -version + swift --version - name: Install Rust run: | rustup toolchain install ${{ env.RUST_VERSION }} --profile minimal From de4d0d4005b7206844e710ca7ef83e3979a86d23 Mon Sep 17 00:00:00 2001 From: lazarevtill Date: Mon, 28 Sep 2026 12:26:11 +0200 Subject: [PATCH 8/8] Reject invalid inventory edits before side effects --- .../ServerGlass.Core.Tests/HostStoreTests.cs | 27 +++++++++++++++++++ apps/windows/ServerGlass.Core/HostStore.cs | 21 ++++++++++++--- apps/windows/ServerGlass/CoreModel.cs | 4 +++ crates/sg-sync/tests/pairing_over_tcp.rs | 8 ++++-- docs/RELIABILITY_AUDIT.md | 17 ++++++++++-- 5 files changed, 69 insertions(+), 8 deletions(-) diff --git a/apps/windows/ServerGlass.Core.Tests/HostStoreTests.cs b/apps/windows/ServerGlass.Core.Tests/HostStoreTests.cs index 0cd51d5..9a891ec 100644 --- a/apps/windows/ServerGlass.Core.Tests/HostStoreTests.cs +++ b/apps/windows/ServerGlass.Core.Tests/HostStoreTests.cs @@ -167,6 +167,8 @@ public void The_known_hosts_path_is_explicit_and_inside_the_data_directory() public void A_corrupt_host_list_is_reported_rather_than_thrown_or_swallowed() { var store = Store(); + var existing = Sample(); + store.SetSecret(existing.Id, "working-password"); Directory.CreateDirectory(_directory); File.WriteAllText(store.HostsPath, "{ this is not the list }"); @@ -175,10 +177,35 @@ public void A_corrupt_host_list_is_reported_rather_than_thrown_or_swallowed() Assert.Empty(store.Load()); Assert.NotNull(reported); + Assert.Throws(() => store.EnsureInventoryReadable()); Assert.Throws(() => store.Save([Sample()])); + Assert.Throws(() => store.SetSecret("new-host", "new-password")); + Assert.Throws(() => store.SetSecret(existing.Id, "replacement")); + Assert.Throws(() => store.Forget(existing)); + Assert.Null(store.Secret("new-host")); + Assert.Equal("working-password", store.Secret(existing.Id)); Assert.Equal("{ this is not the list }", File.ReadAllText(store.HostsPath)); } + [Fact] + public void A_repaired_inventory_can_be_loaded_and_edited_again() + { + var store = Store(); + Directory.CreateDirectory(_directory); + File.WriteAllText(store.HostsPath, "broken"); + Assert.Empty(store.Load()); + Assert.Throws(() => store.SetSecret("new-host", "password")); + + File.WriteAllText(store.HostsPath, "[]"); + Assert.Empty(store.Load()); + store.EnsureInventoryReadable(); + var host = Sample(); + Assert.True(store.SetSecret(host.Id, "password")); + store.Save([host]); + Assert.Equal(host, Assert.Single(store.Load())); + Assert.Equal("password", store.Secret(host.Id)); + } + /// An empty store is an empty list, not a crash on first launch. [Fact] public void A_first_launch_with_no_file_loads_nothing() diff --git a/apps/windows/ServerGlass.Core/HostStore.cs b/apps/windows/ServerGlass.Core/HostStore.cs index 95b1a7c..6cfc350 100644 --- a/apps/windows/ServerGlass.Core/HostStore.cs +++ b/apps/windows/ServerGlass.Core/HostStore.cs @@ -108,10 +108,16 @@ public IReadOnlyList Load() /// Raised when the saved list could not be read. The app shows it; it is not fatal. public event EventHandler? LoadFailed; - public void Save(IReadOnlyList hosts) + /// Reject an edit before its caller changes credentials or live targets. + public void EnsureInventoryReadable() { if (_loadFailed) throw new InvalidOperationException("The unreadable server list has been kept unchanged. Repair it before saving changes."); + } + + public void Save(IReadOnlyList hosts) + { + EnsureInventoryReadable(); System.IO.Directory.CreateDirectory(Directory); // Written to a temporary file and moved into place, so a crash mid-write cannot leave a // half-written list where a complete one used to be. @@ -124,11 +130,18 @@ public void Save(IReadOnlyList hosts) _secrets.Get(hostId, kind); /// false when the credential store refused to keep the secret. - public bool SetSecret(string hostId, string? secret, SecretKind kind = SecretKind.Password) => - _secrets.Set(hostId, kind, secret); + public bool SetSecret(string hostId, string? secret, SecretKind kind = SecretKind.Password) + { + EnsureInventoryReadable(); + return _secrets.Set(hostId, kind, secret); + } /// Erase everything secret belonging to a host. - public void Forget(SavedHost host) => _secrets.Forget(host.Id); + public void Forget(SavedHost host) + { + EnsureInventoryReadable(); + _secrets.Forget(host.Id); + } /// /// Build the config the core wants, pulling the secret out of the credential store at the last diff --git a/apps/windows/ServerGlass/CoreModel.cs b/apps/windows/ServerGlass/CoreModel.cs index a7c3238..479418b 100644 --- a/apps/windows/ServerGlass/CoreModel.cs +++ b/apps/windows/ServerGlass/CoreModel.cs @@ -133,6 +133,9 @@ private Host Adopt(SavedHost saved) public Host Add(SavedHost saved, string? secret, string? keyText) { + // Saving can be forbidden after a failed restore. Refuse before writing credentials or + // adopting a live target, otherwise a reported failure still starts an unsaved server. + Store.EnsureInventoryReadable(); // Secrets first: the config handed to the core is built from them, and a storage refusal // must be reported as a storage refusal rather than as a sign-in failure a screen later. if (!string.IsNullOrEmpty(secret) && !Store.SetSecret(saved.Id, secret)) @@ -155,6 +158,7 @@ public Host Add(SavedHost saved, string? secret, string? keyText) public void Remove(Host host) { + Store.EnsureInventoryReadable(); _core.RemoveTarget(host.TargetId); Store.Forget(host.Saved); Hosts.Remove(host); diff --git a/crates/sg-sync/tests/pairing_over_tcp.rs b/crates/sg-sync/tests/pairing_over_tcp.rs index 44c366b..ad17d89 100644 --- a/crates/sg-sync/tests/pairing_over_tcp.rs +++ b/crates/sg-sync/tests/pairing_over_tcp.rs @@ -61,19 +61,23 @@ async fn a_listener_that_never_completes_the_handshake_does_not_block_later_addr #[tokio::test] async fn a_device_with_a_different_key_than_the_qr_is_rejected() { - use tokio::io::AsyncWriteExt; + use tokio::io::{AsyncReadExt, AsyncWriteExt}; let impostor = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); let (_, offer) = sg_sync::pairing::Handshake::offering(vec![impostor.local_addr().unwrap().to_string()]); let answering = tokio::spawn(async move { let (mut stream, _) = impostor.accept().await.unwrap(); + // Read the client's handshake before closing. Windows resets a socket closed with + // unread request bytes, hiding the deliberately wrong key behind a transport error. + let mut client_key = [0; 32]; + stream.read_exact(&mut client_key).await.unwrap(); stream.write_all(&[1; 32]).await.unwrap(); }); let error = send_transfer(&offer) .await .err() .expect("mismatched QR key accepted"); - assert!(error.to_string().contains("does not match")); + assert!(error.to_string().contains("does not match"), "{error}"); answering.await.unwrap(); } diff --git a/docs/RELIABILITY_AUDIT.md b/docs/RELIABILITY_AUDIT.md index 05bcf38..4240d7c 100644 --- a/docs/RELIABILITY_AUDIT.md +++ b/docs/RELIABILITY_AUDIT.md @@ -54,11 +54,24 @@ device-local credentials, and shared Rust formatting/severity decisions. | Android | Built the arm64 debug APK; all 9 JVM tests passed; installed and launched on the foldable emulator with no recorded crash. This was a fixture launch, not a complete interactive Android UI test. | | Linux | Built in a Rust 1.89 Debian container with GTK/libadwaita; format and Clippy passed with warnings denied; 24 unit tests and 4 live engine tests passed. | | Windows bridge | Built the .NET projects and passed 24 tests against the real native library on macOS, including concurrent disposal. DPAPI tests were excluded because they require Windows. | -| CI configuration | Added the macOS packaging/bridge job and required live fixtures for Linux jobs; YAML parses locally. The updated workflow has not yet run on GitHub. | +| CI configuration | Added the macOS packaging/bridge job and required live fixtures for Linux jobs. Current results are tracked in [PR #9](https://github.com/lazarevtill/ServerGlass/pull/9). | + +## PR review follow-up + +The automated review identified a Windows add-host failure that occurred after credentials and +the live target were already created. Inventory rejection now happens before either mutation; +storage tests verify that existing secrets are preserved, new secrets are refused, and edits +resume after a repaired inventory is loaded. The local C# bridge suite now contains 25 passing tests. + +GitHub's initial Windows job also passed the WinUI build and all 35 tests, including DPAPI. +CI exposed two portability issues: the macOS runner selected Swift 5.10, so the job now selects +Xcode 16.4 on macOS 15; a pairing test closed its mock socket with unread request bytes, which +Windows reset before returning the deliberately wrong key. The mock now reads the handshake +before replying, retaining the key-mismatch assertion. Latest-head results are in the PR. ## Limits and release follow-up -- WinUI build/launch, Windows installation and DPAPI must still be verified on Windows. +- Windows application launch and installation still need interactive verification. - Linux desktop rendering and complete Android interaction need native UI testing. An iPad and physical iOS device were not exercised. - This pass is not proof that the repository contains no remaining bugs. It does not establish